Skip to content

bpo-27645: Supporting native backup facility of SQLite #377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lelit wants to merge 21 commits into from
Closed

bpo-27645: Supporting native backup facility of SQLite #377

Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
d93391d
Expose the SQLite Online Backup API as Connection.backup()
lelit Jul 28, 2016
7b1277a
Preliminary documentation for sqlite3.Connection.backup()
lelit Jul 28, 2016
1258d47
Reduce code nesting depth by using a finally label, as other function…
lelit Mar 1, 2017
d8ea457
Wrap SQLite calls between BEGIN_ALLOW_THREADS and END_ALLOW_THREADS
lelit Mar 1, 2017
35fa0ad
Add an entry in Misc/NEWS related to issue #27645
lelit Mar 1, 2017
c1af5a0
Remove extraneous entry introduced in d4439d6c5b2a67682f3db74d395c603…
lelit Mar 1, 2017
c07d982
Attribute the patch to myself
lelit Mar 1, 2017
cdc7eda
Make pages and progress keyword-only args in sqlite3.Connection.backup()
lelit Mar 1, 2017
6d3eb69
Remove reference to SQLite docs in Connection.backup() documentation
lelit Mar 1, 2017
6fab17f
Use f-string in the sqlite3.Connection.backup() example
lelit Mar 1, 2017
6449f3d
Explicitly mark sqlite3.Connection.backup() as added in v3.7
lelit Mar 1, 2017
f84d375
Reduce chances of cluttering future “git blame”
lelit Mar 1, 2017
e748942
Parametrize the name of the database involved in the backup
lelit Mar 3, 2017
426dad4
Test propagation of exception raised in backup's progress callback
lelit Mar 3, 2017
895726b
Use a better name for variable
lelit Mar 3, 2017
77dfa39
Do not delay next iteration if the result was OK
lelit Mar 3, 2017
71726ba
Omit the backup method when underlying SQLite library is older than 3…
lelit Mar 3, 2017
c4fbc20
Assert that the non-mandatory arguments are keyword-only
lelit Mar 3, 2017
ff1609b
Pass also the current status of the ongoing backup to the progress ca…
lelit Mar 4, 2017
d688f20
Slightly different way handling backup step's error state
lelit Mar 4, 2017
4ed6c31
When an error occurs while the backup is going on, remove the target …
lelit Mar 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Reduce code nesting depth by using a finally label, as other function… 
…s do
  • Loading branch information
@lelit
lelit committed Mar 1, 2017
commit 1258d47ccfab132d89e57e41f56cf287d5b64f47
66 changes: 36 additions & 30 deletions Modules/_sqlite/connection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1483,63 +1483,69 @@ pysqlite_connection_backup(pysqlite_Connection* self, PyObject* args)
char* filename;
int pages = -1;
PyObject* progress = Py_None;
PyObject* retval = NULL;
int rc;
sqlite3 *bckconn;
sqlite3_backup *bckhandle;

if (!PyArg_ParseTuple(args, "s|iO:backup(filename, pages, progress)",
&filename, &pages, &progress)) {
return NULL;
goto finally;
}

if (progress != Py_None && !PyCallable_Check(progress)) {
PyErr_SetString(PyExc_TypeError, "progress argument must be a callable");
return NULL;
goto finally;
}

if (pages == 0) {
pages = -1;
}

rc = sqlite3_open(filename, &bckconn);

@berkerpeksag berkerpeksag Mar 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#359 could be useful here.

@lelit lelit Mar 1, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you are suggesting: should I take the same approach of conditionally using sqlite3_open_v2() instead?

@berkerpeksag berkerpeksag Mar 1, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to say that this should be merged after #359.

@palaviv palaviv Mar 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This don't actually have any effect. sqlite3_open(filename, db) is the same as sqlite3_open_v2(filename, db, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, NULL)

if (rc == SQLITE_OK) {
bckhandle = sqlite3_backup_init(bckconn, "main", self->db, "main");
if (bckhandle) {
do {
rc = sqlite3_backup_step(bckhandle, pages);

if (progress != Py_None) {
if (!PyObject_CallFunction(progress, "ii",
sqlite3_backup_remaining(bckhandle),
sqlite3_backup_pagecount(bckhandle))) {
/* User's callback raised an error: interrupt the loop and
propagate it. */
rc = -1;
}
}
if (rc != SQLITE_OK) {
goto finally;
}

bckhandle = sqlite3_backup_init(bckconn, "main", self->db, "main");

@palaviv palaviv Mar 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't database name be a parameter?

@lelit lelit Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not really the database name: accordingly to the documentation that name denotes the target, that can either be "main" for the main storage, "temp" for the temporary storage or the name of the attached database.
Of course we can easily expose that as a (default) argument, but I did not see as necessary when I coded the feature.

@lelit lelit Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a source_name and a dest_name parameters.

@lelit lelit Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm, further investigation seems to reveal that the dest_name is pointless: attached databases are not a persistent property of a database, so that we cannot allow the following:

bck = sqlite.connect('/tmp/thebackup')
bck.execute(f"ATTACH DATABASE '/tmp/theattachment' AS attached_db")
bck.commit()

db = sqlite.connect('/tmp/database')
db.backup('/tmp/thebackup', dest_name='attached_db')

Changed back to a single name argument to specify the source database...

if (bckhandle) {
do {
rc = sqlite3_backup_step(bckhandle, pages);

/* Sleep for 250ms if there are still further pages to copy */
if (rc == SQLITE_OK || rc == SQLITE_BUSY || rc == SQLITE_LOCKED) {
sqlite3_sleep(250);
if (progress != Py_None) {

@palaviv palaviv Mar 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should call progress only when sqlite3_backup_step return SQLITE_OK

@lelit lelit Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about that: maybe a better option would be pass the result status to the progress callback, so that it can eventually signal the error and do some cleanup (think about closing a progress dialog).

@lelit lelit Mar 4, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the progress callback receives three arguments.

if (!PyObject_CallFunction(progress, "ii",
sqlite3_backup_remaining(bckhandle),
sqlite3_backup_pagecount(bckhandle))) {
/* User's callback raised an error: interrupt the loop and
propagate it. */
rc = -1;

@palaviv palaviv Mar 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add a test for the case that progress raise an error

@lelit lelit Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will do!

@lelit lelit Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
} while (rc == SQLITE_OK || rc == SQLITE_BUSY || rc == SQLITE_LOCKED);
}

sqlite3_backup_finish(bckhandle);
}
/* Sleep for 250ms if there are still further pages to copy */
if (rc == SQLITE_OK || rc == SQLITE_BUSY || rc == SQLITE_LOCKED) {
sqlite3_sleep(250);

@palaviv palaviv Mar 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure we need to sleep when SQLITE_OK is returned? I know this is what is done in the example but we do allow other threads to take the lock between calls to sqlite3_backup_step.
In addition we might want to have some kind of timeout here. We can block forever if someone holds the db lock.

@lelit lelit Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we should: the step function returns OK when it “successfully copies N pages and there are still more pages to be copied”.
Not sure about the timeout: maybe we could check whether there is no progress in, say, X calls to the step function?

@lelit lelit Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reconsidering, I think you're right here: there's no reason to delay next iteration when the result was SQLITE_OK.

}
} while (rc == SQLITE_OK || rc == SQLITE_BUSY || rc == SQLITE_LOCKED);

if (rc != -1) {
rc = _pysqlite_seterror(bckconn, NULL);
}
sqlite3_backup_finish(bckhandle);
}

if (rc != -1) {
rc = _pysqlite_seterror(bckconn, NULL);

@palaviv palaviv Mar 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about this but from the documentation sqlite3_backup_step will not set an error code on error but will return one. I can see that in the example they do the same as here but I am not sure if this is the right behavior. Would we receive SQLITE_NOMEM error from sqlite3_backup_step?

@lelit lelit Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will try to understand the point.

@lelit lelit Mar 4, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to address this concern in my latest patches.

}

sqlite3_close(bckconn);

if (rc != 0) {
/* TODO: should the (probably incomplete/invalid) backup be removed here? */
return NULL;
if (rc == SQLITE_OK) {
Py_INCREF(Py_None);
retval = Py_None;
} else {
Py_RETURN_NONE;
/* TODO: should the (probably incomplete/invalid) backup be removed here? */

@palaviv palaviv Mar 1, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should delete the backup

@lelit lelit Mar 2, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, will do that.

Thanks a lot for the review!

@lelit lelit Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need some help here: I expected to find an abstract/portable PyOS_UnlinkFile(), but couldn't find it.
What is the recommended idiom to remove a file from C? Is the only way to "import" the os module and then PyObject_CallFunction() on its unlink member?

@lelit lelit Mar 4, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the simplest way for now, using unlink(2) from unistd.h. Any hint for a better way is welcome.

}

finally:
return retval;
}

static PyObject *
Expand Down