Skip to content

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

Merged
pablogsal merged 16 commits into
python:mainfrom
erlend-aasland:sqlite-optimise-resets
Sep 21, 2021
Merged

bpo-44958: Only reset sqlite3 statements when needed#27844
pablogsal merged 16 commits into
python:mainfrom
erlend-aasland:sqlite-optimise-resets

Conversation

@erlend-aasland

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

Copy link
Copy Markdown
Contributor

This was referenced Aug 19, 2021
Merged
Closed
@erlend-aasland

erlend-aasland commented Aug 20, 2021

Copy link
Copy Markdown
ContributorAuthor

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

@pablogsalpablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 21, 2021
@bedevere-bot

bedevere-bot commented Aug 21, 2021

Copy link
Copy Markdown

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

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 21, 2021
@erlend-aaslanderlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 22, 2021
@bedevere-bot

bedevere-bot commented Aug 22, 2021

Copy link
Copy Markdown

🤖 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 ":hammer: test-with-buildbots" label again.

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

erlend-aasland commented Aug 22, 2021

Copy link
Copy Markdown
ContributorAuthor

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

@pablogsal

pablogsal commented Aug 25, 2021

Copy link
Copy Markdown
Member

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

@erlend-aasland

erlend-aasland commented Aug 25, 2021

Copy link
Copy Markdown
ContributorAuthor

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

@pablogsalpablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 25, 2021
@bedevere-bot

bedevere-bot commented Aug 25, 2021

Copy link
Copy Markdown

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

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

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

erlend-aasland commented Aug 25, 2021

Copy link
Copy Markdown
ContributorAuthor

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

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

bedevere-bot commented Sep 6, 2021

Copy link
Copy Markdown

🤖 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 ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 6, 2021
@erlend-aasland
erlend-aasland marked this pull request as ready for review September 7, 2021 05:26
@erlend-aasland

erlend-aasland commented Sep 7, 2021

Copy link
Copy Markdown
ContributorAuthor

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.

pablogsal
pablogsal reviewed Sep 19, 2021
View reviewed changes
Comment threadModules/_sqlite/cursor.c
pablogsal
pablogsal requested changes Sep 19, 2021
View reviewed changes

@pablogsalpablogsal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bedevere-bot

bedevere-bot commented Sep 19, 2021

Copy link
Copy Markdown

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.

@pablogsal
pablogsal merged commit 050d103 into python:mainSep 21, 2021
@erlend-aasland

erlend-aasland commented Sep 21, 2021

Copy link
Copy Markdown
ContributorAuthor

Thanks for reviewing, Pablo!

@erlend-aasland
erlend-aasland deleted the sqlite-optimise-resets branch September 21, 2021 11:22
@erlend-aasland

erlend-aasland commented Sep 21, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Sep 21, 2021
Erlend E. 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-aaslanderlend-aasland mentioned this pull request Sep 21, 2021
Merged
miss-islington pushed a commit that referenced this pull request Sep 21, 2021
Erlend Egeberg Aasland
bpo-44958: Fix ref. leak introduced inGH-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 pushed a commit to erlend-aasland/cpython that referenced this pull request Sep 26, 2021
Erlend E. Aasland
bpo-44958: RevertpythonGH-27844
3768af7
This reverts commit 050d103, but keeps
the tests.
pablogsal pushed a commit that referenced this pull request Sep 26, 2021
Erlend Egeberg Aasland
bpo-44958: RevertGH-27844(GH-28574)
7b88f63
This reverts commit 050d103, but keeps
the tests.
@erlend-aaslanderlend-aasland mentioned this pull request May 16, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsalpablogsalpablogsal approved these changes
@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksag
@serhiy-storchakaserhiy-storchakaAwaiting requested review from serhiy-storchaka

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erlend-aasland@bedevere-bot@pablogsal@the-knights-who-say-ni