This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: [sqlite3] refactor pysqlite_statement_create
Type: enhancement Stage: resolved
Components: Extension Modules Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: erlendaasland Nosy List: erlendaasland, pablogsal
Priority: low Keywords: patch

Created on 2021-06-06 22:21 by erlendaasland, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 26566 merged erlendaasland, 2021-06-06 22:26
Messages (7)
msg395224 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-06-06 22:21
Currently, pysqlite_statement_create() approx. looks like this:

1. some sanity checks (type, sql lenght, etc.)
2. allocate (PyObject_GC_New)
3. initialise members
4. determine if statement is a DML statement
5. create the statement (sqlite3_prepare_v2)
6. PyObject_GC_Track
7. check statement return value
8. more sanity checking
9. done!


Suggesting to refactor as this:
1. all sanity checks => early exit on failure
2. create the statement and validate return value
3. determine if statement is a DML statement => no need to do this if statement creation failed
4. allocate
5. initialise members
5. return


This will avoid unneeded allocations/GC tracking, it will avoid unneeded statement creation, and it will be more readable/maintainable.
msg395228 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-06 23:14
>  will avoid unneeded allocations/GC tracking,

I am not sure what you mean, in the happy path you still need to GC track and allocate.
msg395229 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-06-06 23:18
> I am not sure what you mean, in the happy path you still need to GC track and allocate.

Currently, we allocate the object, then try to create the statement using the SQLite API. If we create the statement first, we can do the sanity check on the return value and just return NULL; then we can allocate the Py object and initialise the members.
msg395230 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-06-06 23:20
... to expand: so currently, if statement creation fails, we must deallocate the PyObject.
msg395231 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-06-06 23:27
Ah, I see I formulated myself a bit unclear:
Yes, we need to allocate/track every time. I just propose to do so as late as possible, in order to avoid allocating a PyObject before we know if it is possible to actually create the statement.
msg395232 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-06 23:34
> Yes, we need to allocate/track every time. I just propose to do so as late as possible, in order to avoid allocating a PyObject before we know if it is possible to actually create the statement.

That describes much better the intent :)
msg395334 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-06-08 15:01
New changeset 1c02655fb08043b3027748ca1179c416c21a4277 by Erlend Egeberg Aasland in branch 'main':
bpo-44329: Refactor sqlite3 statement creation (GH-26566)
https://github.com/python/cpython/commit/1c02655fb08043b3027748ca1179c416c21a4277
History
Date User Action Args
2022-04-11 14:59:46adminsetgithub: 88495
2021-06-08 15:57:40erlendaaslandsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2021-06-08 15:01:02pablogsalsetmessages: + msg395334
2021-06-06 23:34:57pablogsalsetmessages: + msg395232
2021-06-06 23:27:30erlendaaslandsetmessages: + msg395231
2021-06-06 23:20:14erlendaaslandsetmessages: + msg395230
2021-06-06 23:18:45erlendaaslandsetmessages: + msg395229
2021-06-06 23:14:53pablogsalsetnosy: + pablogsal
messages: + msg395228
2021-06-06 22:26:04erlendaaslandsetkeywords: + patch
stage: patch review
pull_requests: + pull_request25155
2021-06-06 22:21:32erlendaaslandcreate