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.

Sign up for GitHub

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

gh-88239: Use sqlite3_stmt_busy() to determine if statements are in use #25984

Merged
merged 16 commits into from
Jun 27, 2022
Merged

gh-88239: Use sqlite3_stmt_busy() to determine if statements are in use #25984

merged 16 commits into from
Jun 27, 2022

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented May 8, 2021 •
edited by bedevere-bot
Loading

@erlend-aasland
Copy link
Contributor Author

No user-visible change, so I'm adding the skip news label.

Marking as draft until I've verified all the exit paths in _pysqlite_query_execute().

@erlend-aasland erlend-aasland marked this pull request as draft May 8, 2021 08:11
@erlend-aasland
Copy link
Contributor Author

cc. @corona10

@erlend-aasland
Copy link
Contributor Author

Marking as draft until I've verified all the exit paths in _pysqlite_query_execute().

I've left the exit paths as they are to avoid changing current behaviour. We can rework _pysqlite_query_execute later. There's much to do there :)

@erlend-aasland erlend-aasland marked this pull request as ready for review June 4, 2021 10:41
@erlend-aasland
Copy link
Contributor Author

@pablogsal, are you comfortable with reviewing this, wrt. SQLite API? The change is very straight-forwared, so it should be easy to review.

Here's a link to the relevant API: https://sqlite.org/c3ref/stmt_busy.html

@pablogsal
Copy link
Member

Do we have tests covering this behaviour from before?

@erlend-aasland
Copy link
Contributor Author

Do we have tests covering this behaviour from before?

The if statement in query execute lacks test coverage. I've got one lying around from the sqlite3 coverage issue. I'll add it to the PR once I'm back at my computer. Thanks!

@pablogsal
Copy link
Member

I would like to ame sure that we have coverage for this to ensure that we don't introduce regressions (although we shouldn't if I read this PR correctly).

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jun 4, 2021 •
edited
Loading

I would like to ame sure that we have coverage for this to ensure that we don't introduce regressions (although we shouldn't if I read this PR correctly).

Of course.

BTW, I restored the optimisation of pysqlite_statement_reset: only call sqlite3_reset if needed. Calling it every time would acquire and release the SQLite mutex.

@erlend-aasland erlend-aasland marked this pull request as draft June 4, 2021 19:14
@erlend-aasland erlend-aasland marked this pull request as ready for review June 4, 2021 19:25
@erlend-aasland erlend-aasland reopened this Jun 5, 2021
@erlend-aasland erlend-aasland marked this pull request as draft June 8, 2021 20:29
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Aug 15, 2021 •
edited
Loading

Putting this on hold until we've cleaned up sqlite3_reset usage: see #27844

#27844 is now merged; this PR is no longer on hold.

Erlend E. Aasland added 2 commits August 19, 2021 09:35
Merge branch 'main' into sqlite-stmt-busy
09e44b9
Greatly simplify SQLite statement reset handling:
626e07e 
Instead of resetting a statment on every "exit" path, only reset when
needed:
  1. reset before first sqlite3_step()
  2. on cursor close, reset the statement attached to it
@erlend-aasland erlend-aasland changed the title bpo-44073: Use sqlite3_stmt_busy() to check statement status bpo-44073: Simplify sqlite3 statement handling; only reset statements when needed Aug 19, 2021
@erlend-aasland erlend-aasland changed the title bpo-44073: Simplify sqlite3 statement handling; only reset statements when needed bpo-44073: Use sqlite3_stmt_busy() to determine if we need to reset statements Aug 19, 2021
@erlend-aasland erlend-aasland marked this pull request as ready for review September 21, 2021 14:10
@erlend-aasland
Copy link
Contributor Author

#27844 was reverted, so I'm putting this on hold again.

@erlend-aasland erlend-aasland marked this pull request as draft October 6, 2021 21:39
@erlend-aasland erlend-aasland changed the title bpo-44073: Use sqlite3_stmt_busy() to determine if we need to reset statements gh-88239: Use sqlite3_stmt_busy() to determine if we need to reset statements Apr 10, 2022
@erlend-aasland erlend-aasland mentioned this pull request May 16, 2022
Closed
@erlend-aasland erlend-aasland marked this pull request as ready for review June 23, 2022 12:14
@erlend-aasland erlend-aasland changed the title gh-88239: Use sqlite3_stmt_busy() to determine if we need to reset statements gh-88239: Use sqlite3_stmt_busy() to determine if statements are in use Jun 23, 2022
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 23, 2022
@bedevere-bot
Copy link

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

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 Jun 23, 2022
@erlend-aasland
Copy link
Contributor Author

Buildbots are happy. I'll be merging this to main in a couple of days. This will not be backported.

@erlend-aasland erlend-aasland merged commit f5c85aa into python:main Jun 27, 2022
@erlend-aasland erlend-aasland deleted the sqlite-stmt-busy branch June 27, 2022 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag berkerpeksag is a code owner

@pablogsal pablogsal Awaiting requested review from pablogsal

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[sqlite3] drop statement in_use field in favour of sqlite3_stmt_busy()
4 participants
@erlend-aasland @pablogsal @bedevere-bot @the-knights-who-say-ni