[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend E. Aasland added the comment: > Congratulations, and thanks for your work and determination to get this done > :) Thanks, and thank you for helping out, Petr :) Also a big thanks to Dong-hee, Berker, Serhiy, Pablo, and Victor for reviews and guidance with this "project". --

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: Hooray! Congratulations, and thanks for your work and determination to get this done :) -- ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: New changeset 401272e6e660445d6556d5cd4db88ed4267a50b3 by Erlend Egeberg Aasland in branch 'main': bpo-42064: Adapt `sqlite3` to multi-phase init (PEP 489) (GH-29234) https://github.com/python/cpython/commit/401272e6e660445d6556d5cd4db88ed4267a50b3

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- pull_requests: +27497 pull_request: https://github.com/python/cpython/pull/29234 ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: New changeset 8f24b7dbcbd83311dad510863d8cb41f0e91b464 by Erlend Egeberg Aasland in branch 'main': bpo-42064: Convert `sqlite3` global state to module state (GH-29073) https://github.com/python/cpython/commit/8f24b7dbcbd83311dad510863d8cb41f0e91b464

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- pull_requests: +27341 pull_request: https://github.com/python/cpython/pull/29073 ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: New changeset 09c04e7f0d26f0006964554b6a0caa5ef7f0bd24 by Erlend Egeberg Aasland in branch 'main': bpo-42064: Add module backref to `sqlite3` callback context (GH-28242) https://github.com/python/cpython/commit/09c04e7f0d26f0006964554b6a0caa5ef7f0bd24

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Diego Ramirez : -- nosy: +DiddiLeija ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- pull_requests: +26661 pull_request: https://github.com/python/cpython/pull/28242 ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend E. Aasland added the comment: I did some experiments using the connection object as a "backref" in the callback context, but it seems that the GC does not play well with such ref circles; I ended up with a lot of ref leaks (yes, I modified the traverse and clear slots to visit and

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: New changeset 979336de34e3d3f40cf6e666b72a618f6330f3c1 by Erlend Egeberg Aasland in branch 'main': bpo-42064: Pass module state to trace, progress, and authorizer callbacks (GH-27940)

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: New changeset 0474d06008f8c9eba660ed20d336ffdc5c4db121 by Erlend Egeberg Aasland in branch 'main': bpo-44991: Normalise `sqlite3` callback naming (GH-28088) https://github.com/python/cpython/commit/0474d06008f8c9eba660ed20d336ffdc5c4db121 --

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by STINNER Victor : -- nosy: -vstinner ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend E. Aasland added the comment: >> Would it be sufficient to hold a reference to the connection object? > > Yes Good, that simplifies things. I'll wait with this until we've resolved PR 27940 though. -- ___ Python tracker

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: > Would it be sufficient to hold a reference to the connection object? Yes. -- ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend E. Aasland added the comment: > The state doesn't have a refcount, but it is owned by a module object, so > callback_context should own a reference to the module object. Would it be sufficient to hold a reference to the connection object? The connection holds a reference to the

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: New changeset 01dea5f12b31862999217c091399a318f23b460a by Petr Viktorin in branch 'main': bpo-42064: Offset arguments for PyObject_Vectorcall in the _sqlite module (GH-27931) https://github.com/python/cpython/commit/01dea5f12b31862999217c091399a318f23b460a

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend E. Aasland added the comment: Thanks, good catch! I'll add that after PR 27934 is merged. -- ___ Python tracker ___ ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: Here's a gotcha you might not be aware of: `create_callback_context` stashes away a pointer to `state`. I don't think we can prove that the `state` will always outlive the callback_context after it'll become possible to deallocate the module The state

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- pull_requests: +26385 pull_request: https://github.com/python/cpython/pull/27940 ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Petr Viktorin : -- pull_requests: +26380 pull_request: https://github.com/python/cpython/pull/27931 ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend E. Aasland added the comment: Petr: > I think the module could use a more comprehensive review for GIL handling > [...] I agree. I created bpo-44991 for this. -- ___ Python tracker

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

miss-islington added the comment: New changeset 9ed523159c7ba840dbf403e02498eeae1b5d3ed9 by Erlend Egeberg Aasland in branch 'main': bpo-42064: Pass module state to `sqlite3` UDF callbacks (GH-27456) https://github.com/python/cpython/commit/9ed523159c7ba840dbf403e02498eeae1b5d3ed9

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: I think the module could use a more comprehensive review for GIL handling, rather than doing it piecewise in individual PRs. I recommend that any function passed to SQLite (and only those) should - be named `*_callback`, for clarity - acquire the GIL at

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- pull_requests: +25982 pull_request: https://github.com/python/cpython/pull/27456 ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

miss-islington added the comment: New changeset d542742128b634264d5b6796297613975211b43b by Erlend Egeberg Aasland in branch 'main': bpo-42064: Optimise `sqlite3` state access, part 1 (GH-27273) https://github.com/python/cpython/commit/d542742128b634264d5b6796297613975211b43b --

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: New changeset 890e22957d427ee994b85d62dfe4d5a7cbd34ec5 by Erlend Egeberg Aasland in branch 'main': bpo-42064: Migrate to `sqlite3_create_collation_v2` (GH-27156) https://github.com/python/cpython/commit/890e22957d427ee994b85d62dfe4d5a7cbd34ec5 --

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- pull_requests: +25818 pull_request: https://github.com/python/cpython/pull/27273 ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: New changeset 4c0deb25ac899fbe4da626ce3cb21f204cdd3aa9 by Erlend Egeberg Aasland in branch 'main': bpo-42064: Finalise establishing sqlite3 global state (GH-27155) https://github.com/python/cpython/commit/4c0deb25ac899fbe4da626ce3cb21f204cdd3aa9 --

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- pull_requests: +25693 pull_request: https://github.com/python/cpython/pull/27156 ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- pull_requests: +25692 pull_request: https://github.com/python/cpython/pull/27155 ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

miss-islington added the comment: New changeset 05162993fe62e7fa3acebdd0062586b9bf63d46a by Erlend Egeberg Aasland in branch 'main': bpo-42064: Move `sqlite3` exceptions to global state, part 2 of 2 (GH-26884) https://github.com/python/cpython/commit/05162993fe62e7fa3acebdd0062586b9bf63d46a

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- pull_requests: +25462 pull_request: https://github.com/python/cpython/pull/26884 ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

miss-islington added the comment: New changeset a50e28377bcf37121b55c2de70d95a5386c478f8 by Erlend Egeberg Aasland in branch 'main': bpo-42064: Move `sqlite3` exceptions to global state, part 1 of 2 (GH-26745) https://github.com/python/cpython/commit/a50e28377bcf37121b55c2de70d95a5386c478f8

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Dong-hee Na added the comment: New changeset 019ad62afd20e80c74f879aa716e339b992a0bb9 by Erlend Egeberg Aasland in branch 'main': bpo-42064: Remove stale extern declarations in `sqlite3` headers (GH-26840) https://github.com/python/cpython/commit/019ad62afd20e80c74f879aa716e339b992a0bb9

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- pull_requests: +25421 pull_request: https://github.com/python/cpython/pull/26840 ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- pull_requests: +25330 pull_request: https://github.com/python/cpython/pull/26745 ___ Python tracker ___

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Petr Viktorin added the comment: New changeset 10a5c806d4dec6c342dcc9888fbe4fa1fa9b7a1f by Erlend Egeberg Aasland in branch 'main': bpo-42064: Move sqlite3 types to global state (GH-26537) https://github.com/python/cpython/commit/10a5c806d4dec6c342dcc9888fbe4fa1fa9b7a1f -- nosy:

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Change by Erlend E. Aasland : -- keywords: +patch pull_requests: +25128 stage: -> patch review pull_request: https://github.com/python/cpython/pull/26537 ___ Python tracker

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend E. Aasland added the comment: Global module state has been established by f461a7fc3f8740b9e79e8874175115a3474e5930 (bpo-42862, GH-24203). We can safely migrate static variables into that struct as a next step. -- ___ Python tracker

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend Egeberg Aasland added the comment: FYI, rebased https://github.com/erlend-aasland/cpython/tree/bpo-42064/all onto master, added Petr Viktorin's _PyType_GetModuleByDef() for use in item 7 (module state). I still run into problems in item 8, but I haven't devoted much time for this

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend Egeberg Aasland added the comment: I've narrowed down the traverse problems to Py_VISIT(state->CacheType). Suspecting this has to do with the cyclic references in cache/connection. Preliminary tests with GC tracking `cache->factory` look promising, but this is a part of the C API

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend Egeberg Aasland added the comment: Sorted out item 7 (module state). Item 8 is mostly in place, although I get ref. count errors if I add traverse/clear callbacks, so I've temporarily disabled those. Proof-of-concept of multi-phase init can be found here:

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend Egeberg Aasland added the comment: Connection.__init__() fixed by iterating though tp_bases in order to find the correct base class to load the state from. There should be a better way to do this than to dig through type member internals. --

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend Egeberg Aasland added the comment: Work with item 7 is progressing; see WIP branch for details. Current showstopper is how to fetch module state from Connection.__init__ when it has been overloaded (for example in /Lib/sqlite3/test/factory.py). Guessing that Py_TYPE(self) does not

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend Egeberg Aasland added the comment: FYI, WIP branch for this bpo is here: https://github.com/erlend-aasland/cpython/commits/bpo-42064/all This currently includes items 1 though 6. I'll update the WIP branch with a draft of items 7 and 8, hopefully tomorrow. --

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend Egeberg Aasland added the comment: No, unless we can add `self` as a private context to a sqlite3 database handle, we can't use that shortcut. However, using a custom context struct works, and it's a minor code change. Proof-of-concept implementation diffstat: 2 files changed, 54

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

Erlend Egeberg Aasland added the comment: Instead of using a custom context struct for sqlite3_create_funcion_v2(), we may just use sqlite3_context_db_handle() to get `self`, and then fetch the state using PyType_GetModuleState(Py_TYPE(self)). --

[issue42064] Convert sqlite3 to multi-phase initialisation (PEP 489)

New submission from Erlend Egeberg Aasland : Porting sqlite3 to multi-phase initialisation is non-trivial and must be broken into smaller bpo's, often with multiple PR's per bpo. As far as I can see, this is the task list: 1) Heap types (PEP 384): bpo-41861 2) Argument Clinic: bpo-40956 3)