Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Oct 26, 2021

Use the compile-time selected default SQLite threaded mode to set the
DB-API 2.0 attribute 'threadsafety' during sqlite3 module initialisation.

SQLite to DB-API mappings:

  • SQLITE_THREADSAFE=0 => threadsafety=0 (single-thread mode)
  • SQLITE_THREADSAFE=1 => threadsafety=3 (fully threadsafe)
  • SQLITE_THREADSAFE=2 => threadsafety=1 (threadsafe at module level)

https://bugs.python.org/issue45613

Erlend E. Aasland added 2 commits October 26, 2021 14:26
Use the compile-time selected default SQLite threaded mode to set the DB-API 2.0 attribute 'threadsafety' Mappings: - SQLITE_THREADSAFE=0 => threadsafety=0 - SQLITE_THREADSAFE=1 => threadsafety=3 - SQLITE_THREADSAFE=2 => threadsafety=1
@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Oct 26, 2021

On hold till #29129 is merged. Will rebase and update docs before marking this as ready for review.

@erlend-aasland erlend-aasland marked this pull request as draft October 28, 2021 07:12
@erlend-aasland erlend-aasland marked this pull request as ready for review October 28, 2021 20:46
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 29, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 396abd4 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

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

Testing with buildbots to verify that this approach works across different SQLite versions/configurations. I've verified locally that it works with SQLite 3.7.15 (oldest supported), 3.36.0, and current 3.37.0 (dev).

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

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 686d70f 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 2, 2021
Copy link
Member

@malemburg malemburg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the patch is good to go as-is, without the additional changes you mentioned in the comments. Thanks, Erlend.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Nov 3, 2021

I think the patch is good to go as-is, without the additional changes you mentioned in the comments. Thanks, Erlend.

Thanks! There is one change I'd like to make before merging, though: add a versionchanged annotation to the threadsafety attribute doc.

Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@malemburg do you have anything else you would like here to be addressed?

Edit: ah, I just saw that you approved before. I am landing this then

@pablogsal pablogsal merged commit c273986 into python:main Nov 3, 2021
@erlend-aasland erlend-aasland deleted the sqlite-threadsafety branch November 3, 2021 21:01
@erlend-aasland
Copy link
Contributor Author

Thanks, Marc-Andre & Pablo 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants