bpo-42064: Convert sqlite3 global state to module state

[erlend-aasland]

https://bugs.python.org/issue42064

[encukou]

Nice! The finish line is near!

pysqlite_get_state_by_cls is a bit dangerous: the cls must be exactly a _sqlite3 class, not a subclass (which would be defined in another module).
There are two uses of it, in pysqlite_row_richcompare (where it's incorrect since it uses Py_TYPE(self)), and in pysqlite_connection_close_impl (where it's correct since it uses the defining class).

I suggest removing pysqlite_get_state_by_cls and switching all uses to pysqlite_get_state_by_type. Avoiding the overhead of _PyType_GetModuleByDef is not worth keeping this trap. (And if you call it with the defining class, the overhead shouldn't be noticeable.)

[erlend-aasland]

Nice! The finish line is near!

Yes, it's been a long journey, but I've learned a thing or two while getting here :)

pysqlite_get_state_by_cls is a bit dangerous: the cls must be exactly a _sqlite3 class, not a subclass (which would be defined in another module). There are two uses of it, in pysqlite_row_richcompare (where it's incorrect since it uses Py_TYPE(self)), [...]

Hm, I was under the impression that the left operand was guaranteed to be an instance of the defining class:

https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_richcompare

[encukou]

"an instance of the defining class" roughly means isinstance(self, defining_cls). Here you need type(self) == defining_cls. The two are different if self is an instance of a subclass.

[encukou]

(And any subclass will necessarily be defined in another module, so PyType_GetModule won't return _sqlite3 for it.)

[erlend-aasland]

"an instance of the defining class" roughly means isinstance(self, defining_cls). Here you need type(self) == defining_cls. The two are different if self is an instance of a subclass.

Yep, I know :) I'm not sure where I got the misconception regarding rich compare from, but that does not matter; I'll fix it right away.

[erlend-aasland]

Also, I'm fine with removing pysqlite_get_state_by_cls in favour of pysqlite_get_state_by_type. It's only used twice, and both uses are slow paths (just before raising an exception).

[erlend-aasland]

PTAL, @encukou :)

[erlend-aasland]

FYI, I ran the sqlitesynth benchmark to see if this change introduces any performance regression:

$ python -m pyperf compare_to a.json b.json Mean +- std dev: [a] 5.22 us +- 0.08 us -> [b] 5.20 us +- 0.11 us: 1.00x faster 

• a: vanilla CPython
• b: This PR

[encukou]

Other than this, the change looks good:

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 FreeBSD Non-Debug 3.x has failed when building commit 8f24b7d.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/172/builds/1015) and take a look at the build logs.
4. Check if the failure is related to this commit (8f24b7d) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/172/builds/1015

Summary of the results of the build (if available):

Click to see traceback logs

remote: Enumerating objects: 25, done. remote: Counting objects: 4% (1/25) remote: Counting objects: 8% (2/25) remote: Counting objects: 12% (3/25) remote: Counting objects: 16% (4/25) remote: Counting objects: 20% (5/25) remote: Counting objects: 24% (6/25) remote: Counting objects: 28% (7/25) remote: Counting objects: 32% (8/25) remote: Counting objects: 36% (9/25) remote: Counting objects: 40% (10/25) remote: Counting objects: 44% (11/25) remote: Counting objects: 48% (12/25) remote: Counting objects: 52% (13/25) remote: Counting objects: 56% (14/25) remote: Counting objects: 60% (15/25) remote: Counting objects: 64% (16/25) remote: Counting objects: 68% (17/25) remote: Counting objects: 72% (18/25) remote: Counting objects: 76% (19/25) remote: Counting objects: 80% (20/25) remote: Counting objects: 84% (21/25) remote: Counting objects: 88% (22/25) remote: Counting objects: 92% (23/25) remote: Counting objects: 96% (24/25) remote: Counting objects: 100% (25/25) remote: Counting objects: 100% (25/25), done. remote: Compressing objects: 9% (1/11) remote: Compressing objects: 18% (2/11) remote: Compressing objects: 27% (3/11) remote: Compressing objects: 36% (4/11) remote: Compressing objects: 45% (5/11) remote: Compressing objects: 54% (6/11) remote: Compressing objects: 63% (7/11) remote: Compressing objects: 72% (8/11) remote: Compressing objects: 81% (9/11) remote: Compressing objects: 90% (10/11) remote: Compressing objects: 100% (11/11) remote: Compressing objects: 100% (11/11), done. remote: Total 13 (delta 12), reused 2 (delta 2), pack-reused 0  From https://github.com/python/cpython * branch main -> FETCH_HEAD Note: switching to '8f24b7dbcbd83311dad510863d8cb41f0e91b464'. You are in 'detached HEAD' state. You can look around, make experimental changes and commit them, and you can discard any commits you make in this state without impacting any branches by switching back to a branch. If you want to create a new branch to retain commits you create, you may do so (now or later) by using -c with the switch command. Example: git switch -c <new-branch-name> Or undo this operation with: git switch - Turn off this advice by setting config variable advice.detachedHead to false HEAD is now at 8f24b7dbcb [bpo-42064](https://bugs.python.org/issue42064): Convert `sqlite3` global state to module state (GH-29073) Switched to and reset branch 'main' renaming build/scripts-3.11/pydoc3 to build/scripts-3.11/pydoc3.11 renaming build/scripts-3.11/idle3 to build/scripts-3.11/idle3.11 renaming build/scripts-3.11/2to3 to build/scripts-3.11/2to3-3.11 renaming build/scripts-3.11/pydoc3 to build/scripts-3.11/pydoc3.11 renaming build/scripts-3.11/idle3 to build/scripts-3.11/idle3.11 renaming build/scripts-3.11/2to3 to build/scripts-3.11/2to3-3.11 Cannot open file '/usr/home/buildbot/python/3.x.koobs-freebsd-9e36.nondebug/build/test-results.xml' for upload

[encukou]

The buildbot failed with signal 9 (SIGKILL), before it got to sqlite tests. Most likely unrelated.

[erlend-aasland]

The buildbot failed with signal 9 (SIGKILL), before it got to sqlite tests. Most likely unrelated.

Yes, I've concluded with it being unrelated. Looks like the buildbot was shut down or restarted.