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

[sqlite3] Remove special rollback handling #88258

Closed
erlend-aasland opened this issue May 9, 2021 · 16 comments
Closed

[sqlite3] Remove special rollback handling #88258

erlend-aasland opened this issue May 9, 2021 · 16 comments
Labels
3.11 extension-modules C modules in the Modules dir

Comments

@erlend-aasland
Copy link
Contributor

erlend-aasland commented May 9, 2021

BPO 44092
Nosy @malemburg, @berkerpeksag, @serhiy-storchaka, @animalize, @pablogsal, @miss-islington, @erlend-aasland
PRs
  • bpo-44092: Don't reset statements/cursors before rollback #26026
  • bpo-44092: Remove unused member reset from sqlite3.Cursor #30377
  • bpo-44092: Move What's New entry to where it belongs #30381
  • Files
  • on_conflict_rollback.py
  • 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 2022-01-03.23:51:30.308>
    created_at = <Date 2021-05-09.21:36:02.291>
    labels = ['extension-modules', '3.11']
    title = '[sqlite3] Remove special rollback handling'
    updated_at = <Date 2022-01-04.09:36:38.892>
    user = 'https://github.com/erlend-aasland'

    bugs.python.org fields:

    activity = <Date 2022-01-04.09:36:38.892>
    actor = 'pablogsal'
    assignee = 'none'
    closed = True
    closed_date = <Date 2022-01-03.23:51:30.308>
    closer = 'erlendaasland'
    components = ['Extension Modules']
    creation = <Date 2021-05-09.21:36:02.291>
    creator = 'erlendaasland'
    dependencies = []
    files = ['50481']
    hgrepos = []
    issue_num = 44092
    keywords = ['patch']
    message_count = 16.0
    messages = ['393338', '393411', '393944', '393945', '394405', '407767', '407780', '407784', '407786', '407787', '407788', '407892', '409607', '409633', '409640', '409658']
    nosy_count = 7.0
    nosy_names = ['lemburg', 'berker.peksag', 'serhiy.storchaka', 'malin', 'pablogsal', 'miss-islington', 'erlendaasland']
    pr_nums = ['26026', '30377', '30381']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue44092'
    versions = ['Python 3.11']

    @erlend-aasland
    Copy link
    Contributor Author

    erlend-aasland commented May 9, 2021

    Ref. bpo-33376 and bpo-10513.

    Quoting from the SQLite 3.7.11 changelog[1]:
    "Pending statements no longer block ROLLBACK. Instead, the pending statement will return SQLITE_ABORT upon next access after the ROLLBACK."

    Quoting from the SQLite 3.8.7.2 changelog[2]:
    "Enhance the ROLLBACK command so that pending queries are allowed to continue as long as the schema is unchanged. Formerly, a ROLLBACK would cause all pending queries to fail with an SQLITE_ABORT or SQLITE_ABORT_ROLLBACK error. That error is still returned if the ROLLBACK modifies the schema."

    Quoting from the SQLite docs[3]:
    "In more recent versions of SQLite, the ROLLBACK will proceed and pending statements will often be aborted, causing them to return an SQLITE_ABORT or SQLITE_ABORT_ROLLBACK error. In SQLite version 3.8.8 (2015-01-16) and later, a pending read will continue functioning after the ROLLBACK as long as the ROLLBACK does not modify the database schema."

    I've done some tests with SQLite versions 3.35.4 and 3.7.15 where I've removed the call to pysqlite_do_all_statements() (introduced by [4]) in pysqlite_connection_rollback_impl(), and I've also removed the pysqlite_Cursor.reset member and all of the related code. The test suite passes fine (except for, as expected, the two tests that check for InterfaceError in case of fetch across rollback).

    Do we really need to special case rollbacks anymore? I've tried to come up with tests that prove this approach wrong, but I haven't found any yet.

    [1] https://sqlite.org/changes.html#version_3_7_11
    [2] https://sqlite.org/changes.html#version_3_8_7_2
    [3] https://www.sqlite.org/lang_transaction.html
    [4] ghaering/pysqlite@95f0956

    @erlend-aasland erlend-aasland added 3.11 extension-modules C modules in the Modules dir labels May 9, 2021
    @erlend-aasland
    Copy link
    Contributor Author

    erlend-aasland commented May 10, 2021

    Quoting pysqlite commit 5a009ed message (ghaering/pysqlite@5a009ed):
    "Implemented a function that resets all statements in the connection's
    statement cache. After calling this function it is always possible to
    rollback a transaction or close a connection."

    The commit is from 2005-12-09. SQLite 3.7.11 wasn't released until 2012, so in 2005 any pending statements would block a rollback. I'm guessing commit 5a009ed addressed that issue.

    @erlend-aasland
    Copy link
    Contributor Author

    erlend-aasland commented May 19, 2021

    The effect of PR 26026 is that InterfaceError is no longer raised for fetch across rollback; instead it is up to SQLite how to handle this:

    • for some cases, SQLITE_ABORT or SQLITE_ABORT_ROLLBACK may be returned, which will result in an OperationalError (accompanied by a nice error message provided by SQLite)
    • for other cases, no error is returned; the operation is allowed and succeeds as expected
    • for yet other cases, no error is returned, and the operation was rolled back

    A NEWS entry should mention the change in behaviour, but I can't see how it would break existing projects; the current code disallows fetch across rollback (InterfaceError), so any problematic code would have been found, handled and fixed during debugging.

    @erlend-aasland
    Copy link
    Contributor Author

    erlend-aasland commented May 19, 2021

    I've crafted a number of rollback tests, but it occurred to me that they are simply just testing SQLite behaviour; not sqlite3 behaviour. I had to adjust the tests according to which version of SQLite was used (for example 3.8.7.2 introduced new behaviour). Such tests are bound to break as SQLite evolves. I'm not sure we want such tests in our test suite; it can make the CI fail for completely unrelated PRs. Suggesting to leave detailed rollback testing to SQLite and just keep a couple of basic tests in our suite.

    @erlend-aasland
    Copy link
    Contributor Author

    erlend-aasland commented May 25, 2021

    Berker, does this look ok to you?

    @erlend-aasland erlend-aasland changed the title [sqlite3] consider removing special rollback handling [sqlite3] Remove special rollback handling May 25, 2021
    @erlend-aasland erlend-aasland changed the title [sqlite3] consider removing special rollback handling [sqlite3] Remove special rollback handling May 25, 2021
    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Dec 6, 2021

    I think this change is no problem.
    Erlend E. Aasland's explanation is very clear.

    There is only one situation that a problem may occur. Write code with SQLite 3.8.7.2+ (2014-11-18), and run it on 3.7.15 (2012-12-12) ~ 3.8.7.1-, but this situation may be difficult to happen, we can note this situation in doc.

    More securely, if run on SQLite 3.8.7.1-, and encounter SQLITE_ABORT_ROLLBACK error code, a prompt can be given to explain the reason.

    Also note that the current main branch is buggy. If don't adopt this change or revert this change later, don't forget to fix the bug of msg407185 (pysqlite_Statement.in_use flag is not reset).

    @erlend-aasland
    Copy link
    Contributor Author

    erlend-aasland commented Dec 6, 2021

    I think this change is no problem.

    Thanks, and thank you for looking reviewing this change.

    There is only one situation that a problem may occur. Write code with SQLite
    3.8.7.2+ (2014-11-18), and run it on 3.7.15 (2012-12-12) ~ 3.8.7.1-, but
    this situation may be difficult to happen, we can note this situation in
    doc.

    How realistic is this scenario? If you compile with, for example 3.14.0 or newer, you'd link with sqlite3_trace_v2, not sqlite3_trace, so the loader would prevent you from running with anything pre 3.14. AFAIK, we've never had such problems.

    More securely, if run on SQLite 3.8.7.1-, and encounter
    SQLITE_ABORT_ROLLBACK error code, a prompt can be given to explain the
    reason.

    You already get both an error message, an (extended) error code. That should be sufficient.

    Also note that the current main branch is buggy. If don't adopt this change
    or revert this change later, don't forget to fix the bug of msg407185
    (pysqlite_Statement.in_use flag is not reset).

    It is a change of behaviour of the internal machinery. Does the change lead to wrong results (duplicate rows, wrong rows returned, no rows returned)? Corrupted/garbage data? Non-deterministic behaviour? Does any of the API's provided by sqlite3 not behave according to the documentation anymore?

    NB: I plan to get rid of the in_use flag long before the beta sets in.

    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Dec 6, 2021

    How realistic is this scenario? If you compile with, for example 3.14.0 or
    newer, you'd link with sqlite3_trace_v2, not sqlite3_trace, so the loader
    would prevent you from running with anything pre 3.14. AFAIK, we've never
    had such problems.

    I mean, after this change, different versions of SQLite will behave differently. And give a message for SQLITE_ABORT_ROLLBACK to explain this problem.

    It is a change of behaviour of the internal machinery. Does the change lead
    to wrong results (duplicate rows, wrong rows returned, no rows returned)?
    Corrupted/garbage data? Non-deterministic behaviour? Does any of the API's
    provided by sqlite3 not behave according to the documentation anymore?

    It just leaks resource, apart from this, there seems to be no problem.

    @erlend-aasland
    Copy link
    Contributor Author

    erlend-aasland commented Dec 6, 2021

    I mean, after this change, different versions of SQLite will behave differently.

    Yes, this is explained in the news item:

    Fetch across rollback no longer raises :exc:`~sqlite3.InterfaceError`.
    Instead we leave it to the SQLite library to handle these cases.
    

    And give a message for SQLITE_ABORT_ROLLBACK to explain this problem.

    I'm not sure this is a good idea; I believe letting the underlying SQLite library explain the error is better than trying to deduce stuff based on what was known at compile time and what we know at runtime.

    It just leaks resource, apart from this, there seems to be no problem.

    Can you provide a reproducer? We've run this change through the ref. leak bots, and they are all green, so if there's a ref. leak, the test suite needs improvements.

    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Dec 6, 2021

    Imagine a person write a code with Python 3.11 and SQLite 3.8.7.2+, and then deploying it to Python 3.11 and SQLite 3.8.7.1-, error may occur. However, this situation is difficult to happen.

    Can you provide a reproducer? We've run this change through the ref.
    leak bots, and they are all green, so if there's a ref. leak, the
    test suite needs improvements.

    The statement in cache will be never reused. If you don't mind, it's not a big problem.

    @erlend-aasland
    Copy link
    Contributor Author

    erlend-aasland commented Dec 6, 2021

    Imagine a person write a code with Python 3.11 and SQLite 3.8.7.2+, and then
    deploying it to Python 3.11 and SQLite 3.8.7.1-, error may occur. However,
    this situation is difficult to happen.

    Yes, I also think this is a far fetched case. I'll see if there's an easy way to handle this. If there isn't, I'm not sure the added complexity will be worth it.

    The statement in cache will be never reused.

    That is not necessarily correct; if the "problematic cursor" executes a new statement, the statement in the cache will be reset, thus it can be reused. Also, if the "problematic cursor" is closed (or deleted), the statement can also be reused. But, if the "problematic cursor" just gets stale and is _never_ used again, it may be a performance issue if it is a hot statement. But, as soon as I can land my changes related to the in_use flag, this will not be an issue anyway.

    @animalize
    Copy link
    Mannequin

    animalize mannequin commented Dec 7, 2021

    If the special rollback handling is removed, the behavior of Connection.rollback() and 'ON CONFLICT ROLLBACK' clause will be consistent.
    See attached file on_conflict_rollback.py.

    @pablogsal
    Copy link
    Member

    pablogsal commented Jan 3, 2022

    New changeset 9d6a239 by Erlend Egeberg Aasland in branch 'main':
    bpo-44092: Don't reset statements/cursors before rollback (GH-26026)
    9d6a239

    @erlend-aasland
    Copy link
    Contributor Author

    erlend-aasland commented Jan 3, 2022

    Reopening; there's some clean-up left.

    After #70214, the reset member of pysqlite_Cursor is now unused. We can remove it and all related code. PR coming.

    @miss-islington
    Copy link
    Contributor

    miss-islington commented Jan 3, 2022

    New changeset f1a5844 by Erlend Egeberg Aasland in branch 'main':
    bpo-44092: Remove unused member reset from sqlite3.Cursor (GH-30377)
    f1a5844

    @pablogsal
    Copy link
    Member

    pablogsal commented Jan 4, 2022

    New changeset a09062c by Erlend Egeberg Aasland in branch 'main':
    bpo-44092: Move What's New entry to where it belongs (GH-30381)
    a09062c

    @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.11 extension-modules C modules in the Modules dir
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants