bpo-45040: Simplify sqlite3 transaction control functions#28019
bpo-45040: Simplify sqlite3 transaction control functions#28019pablogsal merged 1 commit intopython:mainfrom
sqlite3 transaction control functions#28019Conversation
| } | ||
|
|
||
| Py_BEGIN_ALLOW_THREADS | ||
| rc = sqlite3_finalize(statement); |
There was a problem hiding this comment.
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.
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_stepalready 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.
Can sqlite3_finalize raise Python errors on its own (via cleanups, callbacks or otherwise)?
There was a problem hiding this comment.
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().
|
🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 0ec285a 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
|
Manually rerunning failed buildbots. See python/buildmaster-config#275 |
|
Thanks for the PR @erlend-aasland ! 🎉 |
https://bugs.python.org/issue45040