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.

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

Merged
merged 1 commit into from Sep 19, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 28, 2021

  • only save/restore thread state once pr. function
  • optimise SQLite C API usage
  • simplify error paths

https://bugs.python.org/issue45040

}

Py_BEGIN_ALLOW_THREADS
rc = sqlite3_finalize(statement);
Copy link
Member

@pablogsal pablogsal Aug 30, 2021

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?

Copy link
Contributor Author

@erlend-aasland erlend-aasland Aug 30, 2021

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

  1. allow threads (unlock GIL), prepare statement, disallow threads (lock GIL)
  2. raise exception and bail on error
  3. unlock GIL, execute SQL statement (sqlite3_step), lock GIL
  4. raise exception and "fall through" on error
  5. unlock GIL, finalise statement (release resources, etc.), lock GIL
  6. raise exception, unless sqlite3_step already did, on error => this is the extra PyErr_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

  1. unlock GIL, create, execute, and finalise SQL statement, lock GIL
  2. raise exception and bail on error

Copy link
Member

@pablogsal pablogsal Aug 30, 2021

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)?

Copy link
Contributor Author

@erlend-aasland erlend-aasland Aug 30, 2021

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().

@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 1, 2021
@bedevere-bot
Copy link

bedevere-bot commented Sep 1, 2021

🤖 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 "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 1, 2021
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Sep 9, 2021

The AMD64 Arch Linux Asan Debug PR is unrelated to this PR.

Manually rerunning failed buildbots. See python/buildmaster-config#275

@pablogsal pablogsal merged commit 771a546 into python:main Sep 19, 2021
74 checks passed
@pablogsal
Copy link
Member

pablogsal commented Sep 19, 2021

Thanks for the PR @erlend-aasland ! 🎉

@erlend-aasland erlend-aasland deleted the sqlite-optimise-tx-fns branch Sep 19, 2021
nsait-linaro pushed a commit to nsait-linaro/cpython that referenced this pull request Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants