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

Jump to bottom

bpo-44958: Only reset sqlite3 statements when needed #27844

Merged
merged 16 commits into from Sep 21, 2021
Merged

bpo-44958: Only reset sqlite3 statements when needed #27844

merged 16 commits into from Sep 21, 2021

Conversation

Reviewers

@pablogsal pablogsal

@berkerpeksag berkerpeksag

@serhiy-storchaka serhiy-storchaka

Assignees
No one assigned
Labels
CLA signed skip news
Projects
None yet
Milestone
No milestone
4 participants
@erlend-aasland @bedevere-bot @pablogsal @the-knights-who-say-ni
@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 19, 2021 •
edited by bedevere-bot

https://bugs.python.org/issue44958
@erlend-aasland
bpo-44958: Only reset sqlite3 statements when needed
99adc05
@erlend-aasland erlend-aasland mentioned this pull request Aug 19, 2021
Draft
@erlend-aasland erlend-aasland mentioned this pull request Aug 19, 2021
Closed
@erlend-aasland
We don't use the return value of pysqlite_statement_reset
2f8cb23
@erlend-aasland
If possible, avoid resetting unused statements
e6e4973
@erlend-aasland
SQLITE_STMTSTATUS_RUN is available from 3.20.0 and onwards
df769c3
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Aug 20, 2021

FYI, tested with SQLite 3.7.15 (minimum required by CPython), 3.8.0, 3.19.0, 3.19.3, 3.20.0 (SQLITE_STMTSTATUS_RUN added), and 3.37.0 (current dev release).

@erlend-aasland
Use sqlite3_stmt pointer iso. deref'ing self when calling sqlite3_reset
cbcd640
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Aug 21, 2021

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit cbcd640 🤖

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

@erlend-aasland
Merge branch 'main' into sqlite-optimise-resets
fe0fd6f
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Aug 22, 2021

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

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

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Aug 22, 2021

The AMD64 Arch Linux Asan Debug PR buildbot failure seems to be unrelated. I see that it's been failing a lot recently.

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Aug 25, 2021

@erlend-aasland You need to rebase the PR on top of main

@erlend-aasland
Sync with 'main' bco. pythonGH-27884
0633095
@erlend-aasland
pysqlite_statement_reset result() is never used => return void
06e242c
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Aug 25, 2021

PTAL. The merge was non-trivial. I suggest running the buildbots again.

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Aug 25, 2021

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 06e242c 🤖

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

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Aug 25, 2021

FYI, we need #27942 for the buildbots to succeed.

@erlend-aasland
Merge branch 'main' into sqlite-optimise-resets
9684593
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Sep 6, 2021

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

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

@erlend-aasland erlend-aasland marked this pull request as ready for review Sep 7, 2021
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Sep 7, 2021

All buildbots but AMD64 Arch Linux Asan Debug PR succeeded. AMD64 Arch Linux Asan Debug PR failed the compile step with RuntimeError: subprocess not supported for isolated subinterpreters, but it has done so the last 18 days.

@erlend-aasland
Merge branch 'main' into sqlite-optimise-resets
41410e0
pablogsal
pablogsal requested changes Sep 19, 2021
View changes
Copy link
Member

@pablogsal pablogsal left a comment

Check https://github.com/python/cpython/pull/27844/files#r711816559

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Sep 19, 2021

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@erlend-aasland
Merge branch 'main' into sqlite-optimise-resets
ebebdb7
@erlend-aasland
Test that datamodifying select queries are marked readonly
c84311e 
python#27844 (comment)
@pablogsal pablogsal merged commit 050d103 into python:main Sep 21, 2021
11 checks passed
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Sep 21, 2021

Thanks for reviewing, Pablo!

@erlend-aasland erlend-aasland deleted the sqlite-optimise-resets branch Sep 21, 2021
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Sep 21, 2021 •
edited

c84311e introduced a leak. I'll try to fix it ASAP. See #28490.

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Sep 21, 2021
@erlend-aasland @erlend-aasland
bpo-44958: Fix ref. leak in test_table_lock_cursor_non_readonly_select()
24e0e12 
Modify managed_connect() helper to support in-memory databases. Use it
for the regression tests added in pythonGH-27844.
@erlend-aasland erlend-aasland mentioned this pull request Sep 21, 2021
Merged
miss-islington pushed a commit that referenced this issue Sep 21, 2021
@erlend-aasland @erlend-aasland
bpo-44958: Fix ref. leak introduced in GH-27844 (GH-28490)
3e3ff09 
Modify managed_connect() helper to support in-memory databases. Use it
for the regression tests added in GH-27844.

Automerge-Triggered-By: GH:pablogsal
erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Sep 26, 2021
@erlend-aasland @erlend-aasland
bpo-44958: Revert pythonGH-27844
3768af7 
This reverts commit 050d103, but keeps
the tests.
pablogsal pushed a commit that referenced this issue Sep 26, 2021
@erlend-aasland @erlend-aasland
bpo-44958: Revert GH-27844 (GH-28574)
7b88f63 
This reverts commit 050d103, but keeps
the tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment