classification
Title: [sqlite3] Active statements are reset twice in _pysqlite_query_execute()
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.10
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, erlendaasland, gstein, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2021-03-01 09:52 by erlendaasland, last changed 2021-05-10 14:03 by erlendaasland.

Files
File name Uploaded Description Edit
fprintf.diff erlendaasland, 2021-05-10 14:00
log.txt erlendaasland, 2021-05-10 14:01
Pull Requests
URL Status Linked Edit
PR 24681 open erlendaasland, 2021-03-01 09:54
PR 26015 gstein, 2021-05-10 12:54
Messages (12)
msg387850 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-03-01 09:52
In _pysqlite_query_execute(), if the cursor already has a statement object, it is reset twice before the cache is queried.
msg387858 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-03-01 12:15
There are three calls of pysqlite_statement_reset() in _pysqlite_query_execute(). And all three of them can be called with the same statement object. But repeated calls are no-ops because pysqlite_statement_reset() clears flag in_use and calls sqlite3_reset() only if it was set before. So additional call of pysqlite_statement_reset() does not harm. You can call it as many times as you want. On other hand removing it can introduce race condition.

Maybe the code could be rewritten in more explicit way and call pysqlite_statement_reset() only when it is necessary, but for now I don't think that just removing one call will make the code better.
msg387859 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-03-01 12:38
> There are three calls of pysqlite_statement_reset() in _pysqlite_query_execute()

Yes, but only two before the cache is queried.

> So additional call of pysqlite_statement_reset() does not harm.

That's true, but removing redundant code makes it easier to read and maintain, IMO.

> On other hand removing it can introduce race condition.

How?

> Maybe the code could be rewritten in more explicit way and call pysqlite_statement_reset() only when it is necessary

Most of _pysqlite_query_execute() could be rewritten in order to make the code clearer. An alternative is to clean it up part by part.
msg387865 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-03-01 13:35
Hm, I guess we could use sqlite3_stmt_busy() instead of the in_use flag.
msg393242 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-05-08 07:48
> Hm, I guess we could use sqlite3_stmt_busy() instead of the in_use flag.

I opened bpo-44073 for this.
msg393382 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2021-05-10 12:39
Serhiy is right. Without a proper research on why it was added in the first place, simply removing the second call won't make code any better and it may introduce regressions (we've already introduced two in 3.10) Maybe doing some digging in pysqlite commits or reaching out to the original author may help.
msg393387 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-05-10 13:19
Relevant historical commits:
- https://github.com/ghaering/pysqlite/commit/a471f0495956c3b8e3f45895b172e522a9ecd683
- https://github.com/ghaering/pysqlite/commit/5a009ed6fb2e90b952438f5786f93cd1e8ac8722
- 191321d11bc7e064e1a07830a43fa600de310e1b (in cpython repo)
msg393394 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-05-10 13:46
Tests that exercise this branch:
Lib/sqlite3/test/dbapi.py: test_in_transaction
Lib/sqlite3/test/dbapi.py: test_last_row_id_insert_o_r
Lib/sqlite3/test/dbapi.py: test_on_conflict_abort_raises_with_explicit_transactions
Lib/sqlite3/test/dbapi.py: test_on_conflict_abort_raises_without_transactions
Lib/sqlite3/test/dbapi.py: test_on_conflict_rollback_with_explicit_transaction
Lib/sqlite3/test/dbapi.py: test_on_conflict_rollback_without_transaction
Lib/sqlite3/test/types.py: test_cursor_description_cte
Lib/sqlite3/test/types.py: test_bool
Lib/sqlite3/test/types.py: test_error_in_conform
Lib/sqlite3/test/userfunctions.py: test_aggr_check_aggr_sum
Lib/sqlite3/test/regression.py: test_column_name_with_spaces
Lib/sqlite3/test/regression.py: test_statement_reset
msg393397 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-05-10 13:59
Adding fprintf's in pysqlite_statement_reset:
diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c
--- a/Modules/_sqlite/statement.c
+++ b/Modules/_sqlite/statement.c
@@ -347,19 +363,23 @@
 int pysqlite_statement_reset(pysqlite_Statement* self)
 {
     int rc;
 
     rc = SQLITE_OK;
 
     if (self->in_use && self->st) {
         Py_BEGIN_ALLOW_THREADS
         rc = sqlite3_reset(self->st);
+        fprintf(stderr, "sqlite3_reset(stmt=%p) => %d: %s\n",
+                self->st, rc, sqlite3_errstr(rc));
         Py_END_ALLOW_THREADS
 
         if (rc == SQLITE_OK) {
             self->in_use = 0;
         }
+    } else {
+        fprintf(stderr, "sqlite3_reset => noop\n");
     }
 
     return rc;
 }
 

In Modules/_sqlite/cursor.c, I've also added a fprintf(stderr, "SECONDRESET: "); before the code in question.


$ ./python.exe -m test test_sqlite  2> log.txt
$ cat log.txt | grep -A1 -B1 SECONDRESET
sqlite3_reset(stmt=0x7fc360177e98) => 0: not an error
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset(stmt=0x7fc3a005f9e8) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3a005ebd8) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3a005f538) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3a005f9e8) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3a005f538) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3a005f9e8) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc35000fe98) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3500107f8) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc350010348) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3500107f8) => 0: not an error
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset(stmt=0x7fc350010348) => 0: not an error
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset(stmt=0x7fc3700287f8) => 0: not an error
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop

--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
sqlite3_reset => noop
--
--
sqlite3_reset => noop
SECONDRESET: sqlite3_reset => noop
msg393398 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-05-10 14:00
fprintf debugging patch added, for reference
msg393399 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-05-10 14:01
Complete fprintf log added, for reference.
msg393400 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-05-10 14:03
Grep for SECONDRESET in log.txt to get the complete "API context".

As far as I can see, there is no harm in removing the redundant reset statement.
History
Date User Action Args
2021-05-10 14:03:50erlendaaslandsetmessages: + msg393400
2021-05-10 14:01:13erlendaaslandsetfiles: + log.txt

messages: + msg393399
2021-05-10 14:00:10erlendaaslandsetfiles: + fprintf.diff

messages: + msg393398
2021-05-10 13:59:15erlendaaslandsetmessages: + msg393397
2021-05-10 13:46:29erlendaaslandsetmessages: + msg393394
2021-05-10 13:19:01erlendaaslandsetmessages: + msg393387
2021-05-10 13:18:44erlendaaslandsetmessages: - msg393386
2021-05-10 13:17:33erlendaaslandsetmessages: + msg393386
2021-05-10 12:54:57gsteinsetnosy: + gstein
pull_requests: + pull_request24667
2021-05-10 12:39:45berker.peksagsetmessages: + msg393382
2021-05-08 07:48:44erlendaaslandsetmessages: + msg393242
2021-03-01 13:35:28erlendaaslandsetmessages: + msg387865
2021-03-01 12:38:31erlendaaslandsetmessages: + msg387859
2021-03-01 12:15:06serhiy.storchakasetmessages: + msg387858
2021-03-01 09:54:16erlendaaslandsetkeywords: + patch
stage: patch review
pull_requests: + pull_request23466
2021-03-01 09:52:49erlendaaslandcreate