bpo-43258: Prevent needless allocation of sqlite3 aggregate context for empty queries

[erlend-aasland]

https://bugs.python.org/issue43258

[erlend-aasland]

@berkerpeksag If you don't mind, I'd like to create a follow-up PR for this issue:

• make the final callback static
• use intermingled declarations to enhance readability

Code coverage is close to 100% for the final callback, so a refactor should be pretty safe.

[berkerpeksag]

• make the final callback static

This sounds good to me. _pysqlite_func_callback() doesn't have static as well.

• use intermingled declarations to enhance readability

I'm not sure about this one. Can you send an inline diff? The function is quite short so I don't see a net gain from a quick look at it.

[berkerpeksag]

Thank you!

[erlend-aasland]

Thanks for reviewing, @berkerpeksag!

_pysqlite_func_callback() doesn't have static as well.

There's a lot of such scoping "errors" in the sqlite3 module. I'm thinking of fixing all in one fairly large PR.

I'll make these two static for now.

• use intermingled declarations to enhance readability

I'm not sure about this one. Can you send an inline diff? The function is quite short so I don't see a net gain from a quick look at it.

Sure! Here it is. Try it out. I believe it's worth it wrt. future maintenance.

diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c
index 977a93cc94..16681f5aea 100644
--- a/Modules/_sqlite/connection.c
+++ b/Modules/_sqlite/connection.c
@@ -699,44 +699,38 @@ static void _pysqlite_step_callback(sqlite3_context *context, int argc, sqlite3_
static void
_pysqlite_final_callback(sqlite3_context *context)
{
- PyObject* function_result;
- PyObject** aggregate_instance;
- _Py_IDENTIFIER(finalize);
- int ok;
- PyObject *exception, *value, *tb;
-
PyGILState_STATE threadstate;
-
threadstate = PyGILState_Ensure();
- aggregate_instance = (PyObject**)sqlite3_aggregate_context(context, 0);
- if (aggregate_instance == NULL) {
+ PyObject **instance = (PyObject**)sqlite3_aggregate_context(context, 0);
+ if (instance == NULL) {
/* No rows matched the query; the step handler was never called. */
goto error;
}
- else if (!*aggregate_instance) {
- /* this branch is executed if there was an exception in the aggregate's
- * __init__ */
-
+ else if (*instance == NULL) {
+ /* There was an exception in the aggregate's __init__ method. */
goto error;
}
/* Keep the exception (if any) of the last call to step() */
+ PyObject *exception, *value, *tb;
PyErr_Fetch(&exception, &value, &tb);
- function_result = _PyObject_CallMethodIdNoArgs(*aggregate_instance, &PyId_finalize);
-
- Py_DECREF(*aggregate_instance);
+ /* Call the method and set result */
+ _Py_IDENTIFIER(finalize);
+ PyObject *res = _PyObject_CallMethodIdNoArgs(*instance, &PyId_finalize);
+ Py_DECREF(*instance);
- ok = 0;
- if (function_result) {
- ok = _pysqlite_set_result(context, function_result) == 0;
- Py_DECREF(function_result);
+ int ok = 0;
+ if (res) {
+ ok = _pysqlite_set_result(context, res) == 0;
+ Py_DECREF(res);
}
if (!ok) {
if (_pysqlite_enable_callback_tracebacks) {
PyErr_Print();
- } else {
+ }
+ else {
PyErr_Clear();
}
sqlite3_result_error(context, "user-defined aggregate's 'finalize' method raised error", -1);

[berkerpeksag]

Sure! Here it is. Try it out. I believe it's worth it wrt. future maintenance.

Sorry, it's pretty much of a rewrite of a pretty short function :) IMO, not worth the code churn. If we ever need to rewrite these functions, it might make sense to do it this way. For now, I prefer keeping it as is.

[erlend-aasland]

Sorry, it's pretty much of a rewrite of a pretty short function :) IMO, not worth the code churn. If we ever need to rewrite these functions, it might make sense to do it this way. For now, I prefer keeping it as is.

I'm not giving up; I'll try to sneak it in later ;)