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.
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
bpo-45040: Simplify sqlite3
transaction control functions
#28019
Conversation
} | ||
|
||
Py_BEGIN_ALLOW_THREADS | ||
rc = sqlite3_finalize(statement); |
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.
Seems that sqlite3_finalize
was protected with the GIL previously and the check for PyErr_Occurred
makes me thing that some callback or similar could end calling Python code. Could you confirm if this is indeed safe?
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.
Hm, sqlite3_finalize
was called outside of the GIL previously, and it still is. Previously, we would release/acquire GIL before/after every SQLite API call; now we release, do all SQLite API calls, then acquire.
Previously
- allow threads (unlock GIL), prepare statement, disallow threads (lock GIL)
- raise exception and bail on error
- unlock GIL, execute SQL statement (
sqlite3_step
), lock GIL - raise exception and "fall through" on error
- unlock GIL, finalise statement (release resources, etc.), lock GIL
- raise exception, unless
sqlite3_step
already did, on error => this is the extraPyErr_Occurred
Since sqlite3_finalize
will pass on the error set by sqlite3_step
, there is no need to check for errors after stepping. We only need to do it after finalising the statement.
Now
- unlock GIL, create, execute, and finalise SQL statement, lock GIL
- raise exception and bail on error
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.
Can sqlite3_finalize
raise Python errors on its own (via cleanups, callbacks or otherwise)?
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.
Not the way we currently use the SQLite API. If we'd added SQLite profile support, sqlite3_finalize
(and other API's) would invoke our _trace_callback()
C callback with argument type
set to SQLITE_TRACE_PROFILE
. We could add an assert(type != SQLITE_TRACE_PROFILE)
and a comment about this to _trace_callback()
.
If you want to schedule another build, you need to add the " |
Manually rerunning failed buildbots. See python/buildmaster-config#275 |
Thanks for the PR @erlend-aasland ! |
https://bugs.python.org/issue45040