Skip to content
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.

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

Supporting native backup facility of SQLite #71832

Closed
lelit mannequin opened this issue Jul 28, 2016 · 44 comments
Closed

Supporting native backup facility of SQLite #71832

lelit mannequin opened this issue Jul 28, 2016 · 44 comments
Labels
3.7 3.8 extension-modules type-feature

Comments

@lelit
Copy link
Mannequin

@lelit lelit mannequin commented Jul 28, 2016

BPO 27645
Nosy @vstinner, @cedk, @bitdancer, @berkerpeksag, @serhiy-storchaka, @lelit, @Vgr255, @AnishShah, @palaviv
PRs
  • #377
  • #4238
  • #6064
  • #6067
  • #6068
  • #6131
  • #6138
  • #6594
  • #12920
  • Files
  • issue27645.patch: Preliminar patch
  • issue27645-doc.patch: Documentation
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2018-03-18.20:01:03.020>
    created_at = <Date 2016-07-28.17:48:18.134>
    labels = ['extension-modules', '3.8', 'type-feature', '3.7']
    title = 'Supporting native backup facility of SQLite'
    updated_at = <Date 2019-04-23.08:26:14.694>
    user = 'https://github.com/lelit'

    bugs.python.org fields:

    activity = <Date 2019-04-23.08:26:14.694>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-03-18.20:01:03.020>
    closer = 'berker.peksag'
    components = ['Extension Modules']
    creation = <Date 2016-07-28.17:48:18.134>
    creator = 'lelit'
    dependencies = []
    files = ['43928', '43930']
    hgrepos = []
    issue_num = 27645
    keywords = ['patch']
    message_count = 44.0
    messages = ['271574', '271575', '271576', '271578', '271580', '271582', '271584', '271676', '272848', '288632', '288723', '288750', '295455', '295472', '298868', '303782', '303789', '304789', '304894', '305444', '307647', '307840', '308022', '308068', '308073', '310669', '313558', '313560', '313561', '313562', '313563', '313564', '313569', '313570', '313571', '313635', '313991', '313994', '313995', '314018', '314026', '314052', '315946', '340701']
    nosy_count = 10.0
    nosy_names = ['ghaering', 'vstinner', 'ced', 'r.david.murray', 'berker.peksag', 'serhiy.storchaka', 'lelit', 'abarry', 'anish.shah', 'palaviv']
    pr_nums = ['377', '4238', '6064', '6067', '6068', '6131', '6138', '6594', '12920']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27645'
    versions = ['Python 3.7', 'Python 3.8']

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Jul 28, 2016

    It would be nice if the sqlite3 stdlib module could expose the SQLite Online Backup API.

    I'm willing to implement it, as encouraged by Paul Moore.

    See also: https://mail.python.org/pipermail/python-dev/2016-July/145570.html

    @lelit lelit mannequin added extension-modules type-feature labels Jul 28, 2016
    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Jul 28, 2016

    Here is a preliminary implementation: lelit@b7456eb

    @Vgr255
    Copy link
    Mannequin

    @Vgr255 Vgr255 mannequin commented Jul 28, 2016

    That's really nice, thank you for doing this! To get your code reviewed, though, you should upload a patch here. With git, you can do 'git diff > my_patch_file_name.patch' in your local repo, then you can upload the resulting file here. If you haven't done so yet, you'll need to sign a contributor agreement as well: https://www.python.org/psf/contrib/contrib-form/ :)

    (Also, you might want to remove the 'index ...' lines in the patch file that git generates for each modified file; since there are three files, there should be three lines to remove. I'm not sure if you really have to, but I always do it just to be sure :)

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Jul 28, 2016

    Thanks for the patch! I haven't had a chance to review the patch yet, but we also need documentation updates to Doc/library/sqlite3.rst.

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Jul 28, 2016

    For the documentation see lelit@bd82f8d or the attached patch.

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Jul 28, 2016

    WRT to the agreement form, I guess I'll have to compile it even if I already contributed to Python decades ago (ObjC, readline, NeXT support...), right?

    Will try to do whatever is needed in the next days...

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Jul 28, 2016

    If you have a copy of your original agreement you could fax it to the PSF, but it is probably easier to just to sign the electronic one.

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Jul 30, 2016

    Ok, the agreement is fullfilled.

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Aug 16, 2016

    I guess the chance of getting this merged before the 3.6 betas is very low, but if there is *anything* I could do to raise it, please tell :-)

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Feb 27, 2017

    Now that we are is officially on GH, would you welcome a PR rebasing this patch on top of the master branch?

    @palaviv
    Copy link
    Mannequin

    @palaviv palaviv mannequin commented Feb 28, 2017

    I actually looked at the patch and have a few comments:

    1. You need to put Py_BEGIN_ALLOW_THREADS and Py_END_ALLOW_THREADS before the sqlite3 calls (especially the sleep).
    2. I think that the pysqlite_connection_backup function will look a lot better if you will have a cleanup/error label.

    I am not a core developer but I think you should open the PR as it will be easier for the CR.

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Mar 1, 2017

    Thank you Aviv, I applied your suggestions and opened a PR.

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Jun 8, 2017

    Is there any chance this could be accepted for Python 3.7?

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Jun 9, 2017

    There's a good chance, yes. You'll have to keep periodically pinging the issue (say once a month :), and if you can specifically talk someone into doing a review your chances go up :) For it to go in we need a review from a core-dev, but one or more reviews from non-core-devs will help it move along as well (less for the core-dev to do when they find the time to do the review).

    @bitdancer bitdancer added the 3.7 label Jun 9, 2017
    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Jul 22, 2017

    Monthly offer to do whatever is needed to easy the path to adoption :-)

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Oct 5, 2017

    I rebased my v2 set of changesets into a new branch: https://github.com/lelit/cpython/tree/sqlite-backup-api-v3

    I really don't know if anybody is interested beyond me, I did everything has been suggested/requested, and honestly I feel a bit discouraged: in the good'n'old days even potentially disrupting and invasive patches of mine have been accepted by the one&only core developer, now for an harmless, tested and documented single new feature an year and half has passed with almost no progress....

    Anyway, I guess that this is my last attempt, let's hope... Let me know if I should close the PR#377 and reopen a new one, or whatever.

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Oct 5, 2017

    If you are talking about Gerhard, if he was still around you'd probably get a similar response, but he hasn't been around much. So this is somewhat of an orphaned module currently and it takes longer for an issue to get traction. One of the drawbacks of the open-source-volunteer model :(

    @cedk
    Copy link
    Mannequin

    @cedk cedk mannequin commented Oct 23, 2017

    I'm using sqlitebck which provides similar functionality but instead of using a file name to store the backup it uses connection instances.
    I find it very useful. Here is my use case: to run tests of an application that requires a database filled, I do a 'copy' of an existing sqlite backup in a ':memory:' database.
    But with the proposed API, it is not possible to use the resulted ':memory:' database because it will be delete when the connection is closed.
    So I propose to add the option to pass a connection instead of a filename.

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Oct 24, 2017

    Reasonable and quite simple to implement: done in commit lelit@960303f

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Nov 2, 2017

    As suggested by Brett Cannon, I closed the original PR#377 and opened a new one against a rebased version of the implementation.

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Dec 5, 2017

    Just to keep the door open, I'm willing to to whatever is needed to see this accepted and merged.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 8, 2017

    It seems to me that the code could be much simpler (and more bugfree) if support only a Connection instance as a target. It is easy to create a Connection instance in Python:

        with sqlite3.connect(filename) as dest:
            source.backup(dest)

    But this would save around 50 lines of complex C code. And I'm not sure this code is correct.

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Dec 11, 2017

    I need advice on Serhiy's proposal of dropping support to plain file name (see also #4238 (comment)).

    Wrt the other point I filed issue bpo-32274.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Dec 11, 2017

    I prefer to keep Connection.backup() simpler. Additional features can be implemented in pure Python.

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Dec 11, 2017

    Thank you Serhiy, ok: will simplify the method, hopefully tomorrow.

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Jan 25, 2018

    I suspect this won't land in 3.7...

    Let me know if I can do something to make that happen, or instead if I should try to rebase the change on top of current master and rectify references to the Python version.

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 10, 2018

    New changeset d7aed41 by Berker Peksag (Emanuele Gaifas) in branch 'master':
    bpo-27645: Add support for native backup facility of SQLite (GH-4238)
    d7aed41

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 10, 2018

    New changeset e8a5a92 by Berker Peksag (Miss Islington (bot)) in branch '3.7':
    bpo-27645: Add support for native backup facility of SQLite (GH-4238)
    e8a5a92

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 10, 2018

    Thanks, Lele.

    Note that Ned gave his permission to get this into 3.7.0b3 at #4238 (comment) We can, of course, still revert it before 3.7.0 final.

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 10, 2018

    From http://buildbot.python.org/all/#/builders/13/builds/808

    ======================================================================
    FAIL: test_bad_target_in_transaction (sqlite3.test.backup.BackupTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Lib/sqlite3/test/backup.py", line 44, in test_bad_target_in_transaction
        self.cx.backup(bck)
    AssertionError: OperationalError not raised

    @berkerpeksag berkerpeksag reopened this Mar 10, 2018
    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 10, 2018

    From test.pythoninfo:

    sqlite3.sqlite_version: 3.8.7.1

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 10, 2018

    AppVeyor:

    sqlite3.sqlite_version: 3.21.0 (passed)

    Travis CI:

    sqlite3.sqlite_version: 3.8.2 (passed)

    http://buildbot.python.org/all/#/builders/88/builds/799

    sqlite3.sqlite_version: 3.8.2 (passed)

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 11, 2018

    Test also passed on my MBP with SQLite 3.22.0 and the following line

        rc = _pysqlite_seterror(bck_conn, NULL);

    returns

    1 (SQLITE_ERROR)
    

    with

    "SQL logic error"
    

    Looging at https://www.sqlite.org/src/artifact?ln=on&name=faf17e60b43233c2, checkReadTransaction() returns

    sqlite3ErrorWithMsg(db, SQLITE_ERROR, "destination database is in use");
    return SQLITE_ERROR;
    

    I've opened PR 6067 to skip the test under SQLite 3.8.7.1 for now.

    Lele, could you take a look at this please? It's almost 4 am here I won't be able to work on this in the next 10-15 hours.

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 11, 2018

    New changeset 7280a4e by Berker Peksag in branch 'master':
    bpo-27645: Skip test_bad_target_in_transaction if SQLite == 3.8.7.1 (GH-6067)
    7280a4e

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 11, 2018

    New changeset c546a62 by Berker Peksag (Miss Islington (bot)) in branch '3.7':
    bpo-27645: Skip test_bad_target_in_transaction if SQLite == 3.8.7.1 (GH-6067)
    c546a62

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Mar 12, 2018

    Sorry, I could not find an easy enough way to compile against SQLite 3.8.7.1, being on Debian sid myself (3.22). I hope to find some time to try harder.

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 17, 2018

    FYI, I will have some time to debug the test failure this weekend. If I (or Lele or someone else) can't find the problem by Monday, I'm going to revert the patch from 3.7 branch (and probably from master too)

    @palaviv
    Copy link
    Mannequin

    @palaviv palaviv mannequin commented Mar 17, 2018

    The problem is that change https://www.sqlite.org/src/info/169b5505498c0a7e was part of sqlite version 3.8.8
    I opened a PR with a fix.

    @lelit
    Copy link
    Mannequin Author

    @lelit lelit mannequin commented Mar 17, 2018

    Thank you Berker&Aviv, I'm sorry I could not find the time to investigate the problem by myself.

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 18, 2018

    New changeset bbf7bb7 by Berker Peksag (Aviv Palivoda) in branch 'master':
    bpo-27645: Fix version number in 'database in transaction' fallback (GH-6131)
    bbf7bb7

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 18, 2018

    New changeset 429ca44 by Berker Peksag (Miss Islington (bot)) in branch '3.7':
    bpo-27645: Fix version number in 'database in transaction' fallback (GH-6131)
    429ca44

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Mar 18, 2018

    Buildbots look happy, closing this one as 'fixed'. Thanks, Aviv!

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Apr 30, 2018

    New changeset ca40501 by Victor Stinner in branch 'master':
    bpo-27645, sqlite: Fix integer overflow on sleep (bpo-6594)
    ca40501

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Apr 23, 2019

    New changeset 8a9a6b4 by Victor Stinner in branch '3.7':
    [3.7] bpo-9566: Fix compiler warnings on Windows (GH-12920)
    8a9a6b4

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 3.8 extension-modules type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants