Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commented May 8, 2021

@erlend-aasland
Copy link
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-aaslanderlend-aasland marked this pull request as draft May 8, 2021 08:11
@erlend-aasland
Copy link
ContributorAuthor

cc. @corona10

@erlend-aasland
Copy link
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-aaslanderlend-aasland marked this pull request as ready for review June 4, 2021 10:41
@erlend-aasland
Copy link
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
Copy link
Member

Do we have tests covering this behaviour from before?

@erlend-aasland
Copy link
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
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
ContributorAuthor

erlend-aasland commented Jun 4, 2021

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-aaslanderlend-aasland marked this pull request as draft June 4, 2021 19:14
@erlend-aaslanderlend-aasland marked this pull request as ready for review June 4, 2021 19:25
@erlend-aaslanderlend-aasland marked this pull request as draft June 8, 2021 20:29
@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commented Aug 15, 2021

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
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-aaslanderlend-aasland marked this pull request as ready for review September 21, 2021 14:10
@erlend-aasland
Copy link
ContributorAuthor

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

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

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

@erlend-aaslanderlend-aasland merged commit f5c85aa into python:mainJun 27, 2022
@erlend-aaslanderlend-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

Labels

Projects

None yet

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