Skip to content

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

Merged
erlend-aasland merged 16 commits into
python:mainfrom
erlend-aasland:sqlite-stmt-busy
Jun 27, 2022
Merged

gh-88239: Use sqlite3_stmt_busy() to determine if statements are in use#25984
erlend-aasland merged 16 commits into
python:mainfrom
erlend-aasland:sqlite-stmt-busy

Conversation

@erlend-aasland

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

Copy link
Copy Markdown
Contributor

@erlend-aasland

erlend-aasland commented May 8, 2021

Copy link
Copy Markdown
ContributorAuthor

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

erlend-aasland commented May 8, 2021

Copy link
Copy Markdown
ContributorAuthor

cc. @corona10

@erlend-aasland

erlend-aasland commented Jun 4, 2021

Copy link
Copy Markdown
ContributorAuthor

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

erlend-aasland commented Jun 4, 2021

Copy link
Copy Markdown
ContributorAuthor

@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

pablogsal commented Jun 4, 2021

Copy link
Copy Markdown
Member

Do we have tests covering this behaviour from before?

@erlend-aasland

erlend-aasland commented Jun 4, 2021

Copy link
Copy Markdown
ContributorAuthor

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

pablogsal commented Jun 4, 2021

Copy link
Copy Markdown
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
erlend-aasland commented Jun 4, 2021
View reviewed changes
Comment threadLib/sqlite3/test/dbapi.py Outdated
@erlend-aasland

erlend-aasland commented Jun 4, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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 marked this pull request as draft June 8, 2021 20:29
@erlend-aasland

erlend-aasland commented Aug 15, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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
Erlend E. Aasland
Merge branch 'main' into sqlite-stmt-busy
09e44b9
Erlend E. Aasland
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-aaslanderlend-aasland changed the title bpo-44073: Use sqlite3_stmt_busy() to check statement statusbpo-44073: Simplify sqlite3 statement handling; only reset statements when neededAug 19, 2021
@erlend-aaslanderlend-aasland changed the title bpo-44073: Simplify sqlite3 statement handling; only reset statements when neededbpo-44073: Use sqlite3_stmt_busy() to determine if we need to reset statementsAug 19, 2021
@erlend-aasland
erlend-aasland marked this pull request as ready for review September 21, 2021 14:10
@erlend-aasland

erlend-aasland commented Oct 6, 2021

Copy link
Copy Markdown
ContributorAuthor

#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-aaslanderlend-aasland changed the title bpo-44073: Use sqlite3_stmt_busy() to determine if we need to reset statementsgh-88239: Use sqlite3_stmt_busy() to determine if we need to reset statementsApr 10, 2022
@erlend-aaslanderlend-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-aaslanderlend-aasland changed the title gh-88239: Use sqlite3_stmt_busy() to determine if we need to reset statementsgh-88239: Use sqlite3_stmt_busy() to determine if statements are in useJun 23, 2022
@erlend-aaslanderlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 23, 2022
@bedevere-bot

bedevere-bot commented Jun 23, 2022

Copy link
Copy Markdown

🤖 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-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 23, 2022
@erlend-aasland

erlend-aasland commented Jun 23, 2022

Copy link
Copy Markdown
ContributorAuthor

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:mainJun 27, 2022
@erlend-aasland
erlend-aasland deleted the sqlite-stmt-busy branch June 27, 2022 07:59
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksag
@pablogsalpablogsalAwaiting 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