bpo-41861: Convert _sqlite3 CursorType and ConnectionType to heap types by erlend-aasland · Pull Request #22478 · python/cpython

@erlend-aasland

Erlend E. Aasland added 2 commits

October 1, 2020 15:27

@vstinner

Merged, thanks. There are two remaining static types?

@erlend-aasland

Merged, thanks. There are two remaining static types?

Thanks! No more remaining types.

@erlend-aasland

Merged, thanks. There are two remaining static types?

BTW, should I add traverse/clear/free methods to the module def before closing the issue?

@vstinner

BTW, should I add traverse/clear/free methods to the module def before closing the issue?

Currently, it's possible to have more than once instance of the _sqlite3 module. If a second instance is created and then cleared, it would clear the shared heap types of the first instance: not good.

It would be better to add a module state and retrieve these types from the module state.

The problem is to retrieve the module state. I suggest you to convert all methods which use these 4 types to Argument Clinic, so later you will be able to use "defining_class" parameter which is a reliable way to get the module state from a type. That's why I asked you to use PyType_FromModuleAndSpec().

Example: _sqlite3.Connection.backup() Python method is implemented as the pysqlite_connection_backup() C function. If you modify the function to use Argument Clinic, you can then get the defining class: then PyType_GetModuleState(cls) gives you the module state.

pysqlite_connection_backup() -> defining_class (cls) -> PyType_GetModuleState(cls) -> module state -> your heap types

Converting _sqlite3 to use a module state will make it safe to be used in Python subinterpreters!

See:

@erlend-aasland

BTW, should I add traverse/clear/free methods to the module def before closing the issue?

Currently, it's possible to have more than once instance of the _sqlite3 module. If a second instance is created and then cleared, it would clear the shared heap types of the first instance: not good.

It would be better to add a module state and retrieve these types from the module state.

I already started experimenting with this. Should I put up what I've got as a draft PR? I guess this goes as a separate issue anyways.

The problem is to retrieve the module state.

I'm painfully aware of this :)

I suggest you to convert all methods which use these 4 types to Argument Clinic, so later you will be able to use "defining_class" parameter which is a reliable way to get the module state from a type. That's why I asked you to use PyType_FromModuleAndSpec().

I think I already put up a PR that converts _sqlite3 to Argument Clinic. I'll rebase onto master and ping you.

Thanks!

@vstinner

a PR that converts _sqlite3 to Argument Clinic

That sounds like a good start! I prefer a PR to convert to Argument Clinic, and then a second one to add a module state.

@erlend-aasland

a PR that converts _sqlite3 to Argument Clinic

That sounds like a good start! I prefer a PR to convert to Argument Clinic, and then a second one to add a module state.

While my other PR's are on hold, I've started with the post-agrument-clinic-step: heap module state (PEP 573). I'm currently basing this branch on top off my work with bpo-40956. Using defining_class makes this very easy, however I need help with resolving two issues:

  1. What to do with the Argument Clinic class definition? We need the module pointer to get to the instances and the types. Quoting from the Argument Clinic docs:
    "When you declare a class, you must also specify two aspects of its type in C: the type declaration you’d use for a pointer to an instance of this class, and a pointer to the PyTypeObject for this class."

  2. What to do with __init__ and __new__? Providing a pointer to the module in the class/type state, would solve it, but that feels backwards and hacky.

UPDATE: Solved like this (excerpt from Argument Clinic for _sqlite3.Connection.__init__):
factory: object(c_default='(PyObject*)((struct pysqlite_state *)PyType_GetModuleState(Py_TYPE(self)))->ConnectionType') = ConnectionType
Is this an accepted solution?

Also, defining_class is not documented. I'll open an issue for that. https://docs.python.org/3.10/howto/clinic.html

@vstinner

@vstinner

I wrote #22712 to "explain my idea". It's a minimum change just to pass a "state" to functions which access the UCD_Type.

@erlend-aasland

I wrote #22712 to "explain my idea". It's a minimum change just to pass a "state" to functions which access the UCD_Type.

Thanks, I'll have a look.

xzy3 pushed a commit to xzy3/cpython that referenced this pull request

Oct 18, 2020

@kylotan kylotan mannequin mentioned this pull request

Sep 19, 2022