Skip to content

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

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

Merged
merged 21 commits into from
Jan 3, 2022

Conversation

erlend-aasland
Copy link
Contributor

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

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
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
Copy link
Contributor Author

erlend-aasland commented May 11, 2021

@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

🤖 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
Copy link
Contributor Author

erlend-aasland commented Nov 10, 2021

@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
Contributor Author

@animalize: would you mind reviewing this?

@ghost
Copy link

ghost commented Nov 17, 2021

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
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
Contributor Author

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

@ghost
Copy link

ghost commented Nov 26, 2021

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
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
Copy link
Contributor Author

erlend-aasland commented Jan 2, 2022

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

@ghost
Copy link

ghost commented Jan 2, 2022

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
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