Skip to content

bpo-44092: Don't reset statements/cursors before rollback#26026

Merged
pablogsal merged 21 commits into
python:mainfrom
erlend-aasland:sqlite-rollback
Jan 3, 2022
Merged

bpo-44092: Don't reset statements/cursors before rollback#26026
pablogsal merged 21 commits into
python:mainfrom
erlend-aasland:sqlite-rollback

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented May 10, 2021

Copy link
Copy Markdown
Contributor

In SQLite versions pre 3.7.11, pending statements would block a
rollback. We now require SQLite 3.7.15, so this workaround can go.

https://bugs.python.org/issue44092

In SQLite versions pre 3.7.11, pending statements would block a
rollback.  This is no longer the case, so remove the workaround.
@erlend-aasland

Copy link
Copy Markdown
Contributor Author

Tests taken from issue 33376

@erlend-aasland erlend-aasland changed the title bpo-44092: Don't reset statements/cursors before rollback [WIP] bpo-44092: Don't reset statements/cursors before rollback May 10, 2021
@erlend-aasland

erlend-aasland commented May 11, 2021

Copy link
Copy Markdown
Contributor Author

@corona10 This change means that InterfaceError is no longer raised for fetch across rollbacks. This is clearly a change in behaviour, but I can't see how it would break existing apps other than peoples fault handlers not being called :) I guess this also needs a What's New item. I'll expand this in the bpo instead of here.

UPDATE: see msg393944

@erlend-aasland erlend-aasland changed the title [WIP] bpo-44092: Don't reset statements/cursors before rollback bpo-44092: Don't reset statements/cursors before rollback May 19, 2021
@erlend-aasland erlend-aasland marked this pull request as ready for review May 19, 2021 10:56
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 8, 2021
@bedevere-bot

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit f7065b1 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 8, 2021
@erlend-aasland

erlend-aasland commented Nov 10, 2021

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka, I'm not sure how to proceed in order to land this. I'm pretty sure this is correct; it works across all supported SQLite versions. I've tried to ping Berker about this, but I haven't gotten any response. Proposal: We merge this to main now; it is still early in 3.11 development, so we have plenty of time to get feedback from the community. For example, the Django people have an extensive sqlite3 test suite, and the've been very quick to report regressions or inaccuracies earlier. If, for some reason, we conclude this is the wrong fix, it will be easy to revert it with good margin before the feature freeze/beta.

See the bpo issue for my notes about this fix.

@erlend-aasland

Copy link
Copy Markdown
Contributor Author

@animalize: would you mind reviewing this?

@ghost

ghost commented Nov 17, 2021

Copy link
Copy Markdown

I will try, I'm not a deep user of SQL, but if you can't find a reviewer I'm glad to try, hope I can review this in 2~4 weeks.

@erlend-aasland

Copy link
Copy Markdown
Contributor Author

I will try, I'm not a deep user of SQL, but if you can't find a reviewer I'm glad to try, hope I can review this in 2~4 weeks.

Great, thanks. You'll find my reasoning in the bpo.

@erlend-aasland

Copy link
Copy Markdown
Contributor Author

@ghaering: Would you mind reviewing this? (A 👎🏻 or 👍🏻 reaction would be 👌🏻). Totally understand if you won't.

@ghost

ghost commented Nov 26, 2021

Copy link
Copy Markdown

It seems reset variable can be removed from pysqlite_Cursor struct.
I'm not a core developer, maybe no one listens to my opinion, but I will help you to find problems.

@erlend-aasland

Copy link
Copy Markdown
Contributor Author

Thanks for helping out, @animalize!

@erlend-aasland erlend-aasland removed the request for review from serhiy-storchaka January 2, 2022 09:24
@erlend-aasland

erlend-aasland commented Jan 2, 2022

Copy link
Copy Markdown
Contributor Author

@pablogsal, it would have been nice to get this into the upcoming alpha :)

@ghost

ghost commented Jan 2, 2022

Copy link
Copy Markdown

It seems this PR can be committed. Then to do subsequent improvement.

@pablogsal pablogsal merged commit 9d6a239 into python:main Jan 3, 2022
@erlend-aasland erlend-aasland deleted the sqlite-rollback branch January 3, 2022 19:31
@erlend-aasland

Copy link
Copy Markdown
Contributor Author

Thank you, Pablo! 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants