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] Improve tuple creation
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.11
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, erlendaasland, mark.dickinson, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2021-04-15 09:23 by erlendaasland, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
patch.diff erlendaasland, 2021-04-15 09:23 harden tuple creation
Pull Requests
URL Status Linked Edit
PR 25421 merged erlendaasland, 2021-04-15 09:28
Messages (6)
msg391122 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-15 09:23
All but one of the PyTuple_SetItem() calls are executed without checking the return value.

Callers:
$ grep -r PyTuple_SetItem Modules/_sqlite 
Modules/_sqlite/connection.c:        PyTuple_SetItem(args, i, cur_py_value);
Modules/_sqlite/cursor.c:        PyTuple_SetItem(row, i, converted);
Modules/_sqlite/cursor.c:    if (PyTuple_SetItem(func_args, 0, Py_NewRef(operation)) != 0) {
Modules/_sqlite/cursor.c:                PyTuple_SetItem(descriptor, 0, column_name);
Modules/_sqlite/cursor.c:                PyTuple_SetItem(descriptor, 1, Py_NewRef(Py_None));
Modules/_sqlite/cursor.c:                PyTuple_SetItem(descriptor, 2, Py_NewRef(Py_None));
Modules/_sqlite/cursor.c:                PyTuple_SetItem(descriptor, 3, Py_NewRef(Py_None));
Modules/_sqlite/cursor.c:                PyTuple_SetItem(descriptor, 4, Py_NewRef(Py_None));
Modules/_sqlite/cursor.c:                PyTuple_SetItem(descriptor, 5, Py_NewRef(Py_None));
Modules/_sqlite/cursor.c:                PyTuple_SetItem(descriptor, 6, Py_NewRef(Py_None));
Modules/_sqlite/cursor.c:                PyTuple_SetItem(self->description, i, descriptor);


All of these are operating on newly created tuples, so I suggest replacing them with PyTuple_SET_ITEM() instead of adding error handling.


For the users in _pysqlite_query_execute() I also suggest to move the tuple creation closer to the code that fills it, in order to minimise the number of decref's needed in case of error.
msg391127 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2021-04-15 10:35
Would the code be cleaner if it used PyTuple_Pack?
msg391128 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-15 10:38
Yes, the _pysqlite_query_execute() case might be cleaner. But I was under the impression that using PyTuple_Pack() was significantly slower. Correct me if I'm wrong.
msg391129 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-04-15 10:49
I've updated my PR to use PyTuple_Pack. Nice reduction in code size. Thanks, Mark!
msg391149 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-04-15 19:25
I considered rewriting this code when touched that files last time (in issue43083). But PyTuple_SetItem() newer fails in these cases, so I left it as is to minimize change.

So it is a pure cosmetic change. I am not opposing it.
msg391680 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2021-04-23 11:21
New changeset e9194ea6eaa18299d6ccbd3555ce150fab0c6184 by Erlend Egeberg Aasland in branch 'master':
bpo-43852: Improve tuple creation in sqlite3 (GH-25421)
https://github.com/python/cpython/commit/e9194ea6eaa18299d6ccbd3555ce150fab0c6184
History
Date User Action Args
2022-04-11 14:59:44adminsetgithub: 88018
2021-04-23 11:21:54berker.peksagsetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: + Python 3.11, - Python 3.10
2021-04-23 11:21:20berker.peksagsetmessages: + msg391680
2021-04-15 19:25:53serhiy.storchakasetmessages: + msg391149
2021-04-15 14:49:39erlendaaslandsettitle: [sqlite3] Harden tuple creation -> [sqlite3] Improve tuple creation
2021-04-15 10:49:10erlendaaslandsetmessages: + msg391129
2021-04-15 10:38:21erlendaaslandsetmessages: + msg391128
2021-04-15 10:35:15mark.dickinsonsetnosy: + mark.dickinson
messages: + msg391127
2021-04-15 09:28:09erlendaaslandsetstage: patch review
pull_requests: + pull_request24153
2021-04-15 09:23:35erlendaaslandcreate