bpo-27645: Supporting native backup facility of SQLite by lelit · Pull Request #4238 · python/cpython

lelit · 2017-11-02T17:54:59Z

This is a new version of PR #377, rebased on more recent master to solve the conflicts on the old NEWS file.

It implements a new requested feature since last review.

See https://bugs.python.org/issue27645

https://bugs.python.org/issue27645

See issue python#27645.

…s do

Suggested by Aviv Palivoda.

When sqlite3_backup_step() returns OK it means that it was able to do it's work but there are further pages to be copied: in such case it's better to immediately start the next iteration, without opening a window that allows other threads to go on and possibly locking the database. Suggested by Aviv Palivoda.

….6.11

…llback Add new SQLITE_DONE integer constant to the module, that could be used by the callback to determine whether the backup is terminated.

This should address Aviv's concerns about how the sqlite3_backup_step() errors are handled.

…file

…onnection

This is needed for SQLite < 3.8.7.

lelit · 2017-11-02T18:38:44Z

Compilation fails on Windows, due to misdeclaration of unlink(). I call that function to remove an incomplete backup when an error happens: I looked around in the sources and couldn't figure out a portable way to do that. Any hint?

tiran · 2017-12-05T08:30:03Z

+#ifdef HAVE_UNISTD_H
+#include <unistd.h>
+#else
+extern int unlink(const char *);


This won't work under Windows. You have to use DeleteFileW, see posixmodule.c

bedevere-bot · 2017-12-12T12:35:17Z

Thanks for making the requested changes!

@berkerpeksag, @tiran: please review the changes made to this pull request.

lelit · 2017-12-22T19:22:16Z

Is there anything left to do on my part?

lelit · 2018-01-09T16:07:14Z

Will this be merged before 3.7b1?

ned-deily · 2018-03-09T21:16:45Z

@tiran and @berkerpeksag, looks this PR is waiting on your reviews of changes. Is there anything else holding this up? It seems like a lot of work went into this PR over a long period and it is quite self-contained. If everyone agrees it is ready to be merged into master in the next few days and if the buildbots are happy with it, I might be persuaded to allow it into 3.7.0b3 as well.

lelit · 2018-03-10T12:16:56Z

👏

berkerpeksag · 2018-03-10T15:54:01Z

Sorry, I've been busy with family stuff and I will take a look at this PR today or tomorrow.

berkerpeksag · 2018-03-10T18:52:54Z

This looks pretty good to me. I've rebased @lelit's branch, made some cosmetic changes, and fixed a segfault (824d5f8)

About supporting a more recent SQLite version as minimum: Last time I've used a newer API (I documented the minimum SQLite version) we got several reports saying "we can't compile Python with SQLite X.Y.Z anymore." Perhaps we can use the death of Python 2.7 as an opportunity and get rid of most of the compatibility code in Python 3.8 or Python 3.9?

berkerpeksag · 2018-03-10T19:22:30Z

@ned-deily

If everyone agrees it is ready to be merged into master in the next few days and if the buildbots are happy with it, I might be persuaded to allow it into 3.7.0b3 as well.

I'm planning to merge this in an hour or two. I've changed the versionadded marker to 3.8 but should I revert it back to 3.7?

ned-deily · 2018-03-10T20:14:18Z

@berkerpeksag, Thanks for taking this on! Yeah, let's go crazy and change it to 3.7. We can always change it later if, for some reason, it doesn't make 3.7.

berkerpeksag · 2018-03-10T20:48:07Z

@ned-deily changed versionadded to 3.7 and added an entry to Doc/whatsnew/3.7.rst. What's the current process for getting something in after b1? Do I need to open a PR against the 3.7 branch or some special branch?

ned-deily · 2018-03-10T20:49:51Z

@berkerpeksag, you can just set the "need backport to 3.7" label to start an auto backport to 3.7

miss-islington · 2018-03-10T22:08:34Z

Thanks @lelit for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

…-4238) (cherry picked from commit d7aed41) Co-authored-by: Emanuele Gaifas <lelegaifax@gmail.com>

bedevere-bot · 2018-03-10T22:09:49Z

GH-6064 is a backport of this pull request to the 3.7 branch.

berkerpeksag · 2018-03-10T22:15:25Z

@lelit thank you for your patience! I know that this was a frustrating experience for you, but I hope you are going to continue contributing to Python :)

(cherry picked from commit d7aed41) Co-authored-by: Emanuele Gaifas <lelegaifax@gmail.com>

lelit · 2018-03-10T23:06:33Z

@berkerpeksag thank you and don't worry, I've been around for enough time to know how it goes and I will surely keep that for the remaining half of my life 😃

…-4238)

lelit added 22 commits October 5, 2017 20:31

Expose the SQLite Online Backup API as Connection.backup()
0c9a4e5
See issue python#27645.

Preliminary documentation for sqlite3.Connection.backup()
624849a

Reduce code nesting depth by using a finally label, as other function…
b17e572
…s do

Wrap SQLite calls between BEGIN_ALLOW_THREADS and END_ALLOW_THREADS
3a64654
Suggested by Aviv Palivoda.

Make pages and progress keyword-only args in sqlite3.Connection.backup()
8220ed6

Remove reference to SQLite docs in Connection.backup() documentation
f7c6fc5

Use f-string in the sqlite3.Connection.backup() example
df607df

Explicitly mark sqlite3.Connection.backup() as added in v3.7
169369b

Reduce chances of cluttering future “git blame”
7ee5341

Parametrize the name of the database involved in the backup
f88cd12
Suggested by Aviv Palivoda.

Test propagation of exception raised in backup's progress callback
7ee018c

Use a better name for variable
21bfc82

Omit the backup method when underlying SQLite library is older than 3…
09407c8
….6.11

Assert that the non-mandatory arguments are keyword-only
f99e65e

Pass also the current status of the ongoing backup to the progress ca…
aaa1508
…llback Add new SQLITE_DONE integer constant to the module, that could be used by the callback to determine whether the backup is terminated.

Slightly different way handling backup step's error state
13de3a1
This should address Aviv's concerns about how the sqlite3_backup_step() errors are handled.

When an error occurs while the backup is going on, remove the target …
9b2f47a
…file

Add NEWS entry for issue 27645
ce55873

New argument "sleep" to specify a different number of milliseconds
5a08168

Allow to copy the database to either an external file or to another C…
960303f
…onnection

Check that target backup DB is not in transaction
4bd0b3e
This is needed for SQLite < 3.8.7.

bedevere-bot added the awaiting review label Nov 2, 2017

the-knights-who-say-ni added the CLA signed label Nov 2, 2017

lelit mentioned this pull request Nov 2, 2017
bpo-27645: Supporting native backup facility of SQLite #377
Closed

brettcannon changed the title ~~Supporting native backup facility of SQLite~~bpo-27645: Supporting native backup facility of SQLiteNov 2, 2017

tiran requested changes Dec 5, 2017
View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Dec 5, 2017

berkerpeksag added 3 commits March 10, 2018 21:33

Merge remote-tracking branch 'upstream/master' into pr/4238
32285b1

Cosmetic fixes
7943561

Fix segfault in test_bad_target_closed_connection
824d5f8

berkerpeksag approved these changes Mar 10, 2018
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Mar 10, 2018

Relax an assert
6fd854a

This will be included in 3.7
57f49c6

berkerpeksag added the needs backport to 3.7 label Mar 10, 2018

berkerpeksag merged commit d7aed41 into python:masterMar 10, 2018

bedevere-bot removed the awaiting merge label Mar 10, 2018

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Mar 10, 2018
bpo-27645: Add support for native backup facility of SQLite (pythonGH…
4067bad
…-4238) (cherry picked from commit d7aed41) Co-authored-by: Emanuele Gaifas <lelegaifax@gmail.com>

bedevere-bot removed the needs backport to 3.7 label Mar 10, 2018

berkerpeksag pushed a commit that referenced this pull request Mar 10, 2018
bpo-27645: Add support for native backup facility of SQLite (GH-4238)
e8a5a92
(cherry picked from commit d7aed41) Co-authored-by: Emanuele Gaifas <lelegaifax@gmail.com>

jo2y pushed a commit to jo2y/cpython that referenced this pull request Mar 23, 2018
bpo-27645: Add support for native backup facility of SQLite (pythonGH…
f985dde
…-4238)

Uh oh!

Uh oh!

Conversation

lelit commented Nov 2, 2017• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lelit commented Nov 2, 2017

Uh oh!

tiranDec 5, 2017

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 12, 2017

Uh oh!

lelit commented Dec 22, 2017

Uh oh!

lelit commented Jan 9, 2018

Uh oh!

ned-deily commented Mar 9, 2018

Uh oh!

lelit commented Mar 10, 2018

Uh oh!

berkerpeksag commented Mar 10, 2018

Uh oh!

berkerpeksag commented Mar 10, 2018

Uh oh!

berkerpeksag commented Mar 10, 2018

Uh oh!

ned-deily commented Mar 10, 2018

Uh oh!

berkerpeksag commented Mar 10, 2018

Uh oh!

ned-deily commented Mar 10, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miss-islington commented Mar 10, 2018

Uh oh!

bedevere-bot commented Mar 10, 2018

Uh oh!

berkerpeksag commented Mar 10, 2018

Uh oh!

lelit commented Mar 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lelit commented Nov 2, 2017•
edited by bedevere-bot
Loading

ned-deily commented Mar 10, 2018•
edited
Loading