GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-44958: Only reset `sqlite3` statements when needed by erlend-aasland · Pull Request #27844 · python/cpython

Conversation

@erlend-aasland

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue44958

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Erlend E. Aasland
bpo-44958: Only reset sqlite3 statements when needed
99adc05
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner August 19, 2021 20:06
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 19, 2021
@bedevere-bot bedevere-bot added the awaiting review label Aug 19, 2021
@erlend-aasland
erlend-aasland requested review from pablogsal and serhiy-storchaka August 19, 2021 20:07
@erlend-aasland erlend-aasland added the skip news label Aug 19, 2021
This was referenced Aug 19, 2021
gh-88239: Use sqlite3_stmt_busy() to determine if statements are in use #25984
Merged
bpo-43350: Don't reset active statements twice in _pysqlite_query_execute() #24681
Closed
Erlend E. Aasland added 3 commits August 20, 2021 15:14
Erlend E. Aasland
We don't use the return value of pysqlite_statement_reset
2f8cb23
Erlend E. Aasland
If possible, avoid resetting unused statements
e6e4973
Erlend E. Aasland
SQLITE_STMTSTATUS_RUN is available from 3.20.0 and onwards
df769c3
@erlend-aasland

erlend-aasland commented Aug 20, 2021

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Erlend E. Aasland
Use sqlite3_stmt pointer iso. deref'ing self when calling sqlite3_reset
cbcd640
@pablogsal pablogsal 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Aug 21, 2021
Erlend E. Aasland
Merge branch 'main' into sqlite-optimise-resets
fe0fd6f
@erlend-aasland erlend-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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-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
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Erlend E. Aasland added 2 commits August 25, 2021 12:44
Erlend E. Aasland
Sync with 'main' bco. pythonGH-27884
0633095
Erlend E. Aasland
pysqlite_statement_reset result() is never used => return void
06e242c
@erlend-aasland

erlend-aasland commented Aug 25, 2021

Copy link
Copy Markdown
Contributor Author

PTAL. The merge was non-trivial. I suggest running the buildbots again.
pablogsal reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal pablogsal 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-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
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland erlend-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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-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
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Erlend E. Aasland
Merge branch 'main' into sqlite-optimise-resets
41410e0
pablogsal
pablogsal approved these changes Sep 19, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Sep 19, 2021
pablogsal
pablogsal reviewed Sep 19, 2021
View reviewed changes
Comment thread Modules/_sqlite/cursor.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal requested changes Sep 19, 2021
View reviewed changes

@pablogsal pablogsal left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting merge labels Sep 19, 2021
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Erlend E. Aasland added 2 commits September 21, 2021 00:38
Erlend E. Aasland
Merge branch 'main' into sqlite-optimise-resets
ebebdb7
Erlend E. Aasland
Test that datamodifying select queries are marked readonly
c84311e 
python#27844 (comment)
pablogsal
pablogsal approved these changes Sep 21, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Sep 21, 2021
@pablogsal
pablogsal merged commit 050d103 into python:main Sep 21, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 21, 2021
@erlend-aasland

erlend-aasland commented Sep 21, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for reviewing, Pablo!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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-aasland erlend-aasland mentioned this pull request Sep 21, 2021
bpo-44958: Fix ref. leak introduced in GH-27844 #28490
Merged
miss-islington pushed a commit that referenced this pull request Sep 21, 2021
Erlend Egeberg 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 pushed a commit to erlend-aasland/cpython that referenced this pull request Sep 26, 2021
Erlend E. Aasland
bpo-44958: Revert pythonGH-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: Revert GH-27844 (GH-28574)
7b88f63 
This reverts commit 050d103, but keeps
the tests.
@erlend-aasland erlend-aasland mentioned this pull request May 16, 2022
[sqlite3] only reset statements when needed #89121
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal pablogsal approved these changes
@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@erlend-aasland @bedevere-bot @pablogsal @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.