From 99adc054bbe35a9f681cf3e39b631d9140028b0a Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Thu, 19 Aug 2021 22:06:23 +0200 Subject: [PATCH 01/10] bpo-44958: Only reset sqlite3 statements when needed --- Modules/_sqlite/cursor.c | 40 +++++++--------------------------------- 1 file changed, 7 insertions(+), 33 deletions(-) diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 7308f3062da4b9..ccf9dcf829622c 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -106,11 +106,7 @@ cursor_clear(pysqlite_Cursor *self) Py_CLEAR(self->row_cast_map); Py_CLEAR(self->lastrowid); Py_CLEAR(self->row_factory); - if (self->statement) { - /* Reset the statement if the user has not closed the cursor */ - pysqlite_statement_reset(self->statement); - Py_CLEAR(self->statement); - } + Py_CLEAR(self->statement); Py_CLEAR(self->next_row); return 0; @@ -124,6 +120,9 @@ cursor_dealloc(pysqlite_Cursor *self) if (self->in_weakreflist != NULL) { PyObject_ClearWeakRefs((PyObject*)self); } + if (self->statement) { + pysqlite_statement_reset(self->statement); + } tp->tp_clear((PyObject *)self); tp->tp_free(self); Py_DECREF(tp); @@ -528,20 +527,11 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation } } - if (self->statement != NULL) { - /* There is an active statement */ - pysqlite_statement_reset(self->statement); - } - /* reset description and rowcount */ Py_INCREF(Py_None); Py_SETREF(self->description, Py_None); self->rowcount = 0L; - if (self->statement) { - (void)pysqlite_statement_reset(self->statement); - } - PyObject *stmt = get_statement_from_cache(self, operation); Py_XSETREF(self->statement, (pysqlite_Statement *)stmt); if (!self->statement) { @@ -555,8 +545,6 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation goto error; } } - - pysqlite_statement_reset(self->statement); pysqlite_statement_mark_dirty(self->statement); /* We start a transaction implicitly before a DML statement. @@ -577,6 +565,7 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation break; } + pysqlite_statement_reset(self->statement); pysqlite_statement_mark_dirty(self->statement); pysqlite_statement_bind_parameters(state, self->statement, parameters); @@ -594,7 +583,6 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation PyErr_Clear(); } } - (void)pysqlite_statement_reset(self->statement); _pysqlite_seterror(state, self->connection->db); goto error; } @@ -664,13 +652,9 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation if (self->next_row == NULL) goto error; } else if (rc == SQLITE_DONE && !multiple) { - pysqlite_statement_reset(self->statement); Py_CLEAR(self->statement); } - if (multiple) { - pysqlite_statement_reset(self->statement); - } Py_XDECREF(parameters); } @@ -837,10 +821,7 @@ pysqlite_cursor_iternext(pysqlite_Cursor *self) } if (!self->next_row) { - if (self->statement) { - (void)pysqlite_statement_reset(self->statement); - Py_CLEAR(self->statement); - } + Py_CLEAR(self->statement); return NULL; } @@ -862,12 +843,10 @@ pysqlite_cursor_iternext(pysqlite_Cursor *self) if (self->statement) { rc = pysqlite_step(self->statement->st); if (PyErr_Occurred()) { - (void)pysqlite_statement_reset(self->statement); Py_DECREF(next_row); return NULL; } if (rc != SQLITE_DONE && rc != SQLITE_ROW) { - (void)pysqlite_statement_reset(self->statement); Py_DECREF(next_row); _pysqlite_seterror(self->connection->state, self->connection->db); return NULL; @@ -876,7 +855,6 @@ pysqlite_cursor_iternext(pysqlite_Cursor *self) if (rc == SQLITE_ROW) { self->next_row = _pysqlite_fetch_one_row(self); if (self->next_row == NULL) { - (void)pysqlite_statement_reset(self->statement); return NULL; } } @@ -1037,11 +1015,7 @@ pysqlite_cursor_close_impl(pysqlite_Cursor *self, PyTypeObject *cls) return NULL; } - if (self->statement) { - (void)pysqlite_statement_reset(self->statement); - Py_CLEAR(self->statement); - } - + Py_CLEAR(self->statement); self->closed = 1; Py_RETURN_NONE; From 2f8cb23768c8e33aec76c58cb86601632e5a85cc Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 20 Aug 2021 15:14:46 +0200 Subject: [PATCH 02/10] We don't use the return value of pysqlite_statement_reset --- Modules/_sqlite/cursor.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index ccf9dcf829622c..9e0d733585b5c0 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -121,7 +121,7 @@ cursor_dealloc(pysqlite_Cursor *self) PyObject_ClearWeakRefs((PyObject*)self); } if (self->statement) { - pysqlite_statement_reset(self->statement); + (void)pysqlite_statement_reset(self->statement); } tp->tp_clear((PyObject *)self); tp->tp_free(self); @@ -565,7 +565,7 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation break; } - pysqlite_statement_reset(self->statement); + (void)pysqlite_statement_reset(self->statement); pysqlite_statement_mark_dirty(self->statement); pysqlite_statement_bind_parameters(state, self->statement, parameters); From e6e4973f08cc77fbdd6abf955d6e42104f6970f5 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 20 Aug 2021 15:15:25 +0200 Subject: [PATCH 03/10] If possible, avoid resetting unused statements --- Modules/_sqlite/statement.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index b20c91da3179c4..c0240d1cc70232 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -362,20 +362,28 @@ pysqlite_statement_bind_parameters(pysqlite_state *state, int pysqlite_statement_reset(pysqlite_Statement* self) { - int rc; + sqlite3_stmt *stmt = self->st; + if (stmt == NULL || self->in_use == 0) { + return SQLITE_OK; + } - rc = SQLITE_OK; +#if SQLITE_VERSION_NUMBER > 3020000 + /* Check if the statement has been run (that is, sqlite3_step() has been + * called at least once). Third parameter is non-zero in order to reset the + * run count. */ + if (sqlite3_stmt_status(stmt, SQLITE_STMTSTATUS_RUN, 1) == 0) { + return SQLITE_OK; + } +#endif - if (self->in_use && self->st) { - Py_BEGIN_ALLOW_THREADS - rc = sqlite3_reset(self->st); - Py_END_ALLOW_THREADS + int rc; + Py_BEGIN_ALLOW_THREADS + rc = sqlite3_reset(self->st); + Py_END_ALLOW_THREADS - if (rc == SQLITE_OK) { - self->in_use = 0; - } + if (rc == SQLITE_OK) { + self->in_use = 0; } - return rc; } From df769c32595711a5b5453773b29cfd4b0777d115 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 20 Aug 2021 15:26:50 +0200 Subject: [PATCH 04/10] SQLITE_STMTSTATUS_RUN is available from 3.20.0 and onwards --- Modules/_sqlite/statement.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index c0240d1cc70232..92183cf31e80fd 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -367,7 +367,7 @@ int pysqlite_statement_reset(pysqlite_Statement* self) return SQLITE_OK; } -#if SQLITE_VERSION_NUMBER > 3020000 +#if SQLITE_VERSION_NUMBER >= 3020000 /* Check if the statement has been run (that is, sqlite3_step() has been * called at least once). Third parameter is non-zero in order to reset the * run count. */ From cbcd64085536890a1477050402bb22f86aaa6e2a Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 20 Aug 2021 20:39:56 +0200 Subject: [PATCH 05/10] Use sqlite3_stmt pointer iso. deref'ing self when calling sqlite3_reset --- Modules/_sqlite/statement.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index 92183cf31e80fd..25cc7ba8d077e2 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -378,7 +378,7 @@ int pysqlite_statement_reset(pysqlite_Statement* self) int rc; Py_BEGIN_ALLOW_THREADS - rc = sqlite3_reset(self->st); + rc = sqlite3_reset(stmt); Py_END_ALLOW_THREADS if (rc == SQLITE_OK) { From 06e242c37686d4a8b33979552481821bfb701a25 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Wed, 25 Aug 2021 12:55:23 +0200 Subject: [PATCH 06/10] pysqlite_statement_reset result() is never used => return void --- Modules/_sqlite/cursor.c | 4 ++-- Modules/_sqlite/statement.c | 8 ++++---- Modules/_sqlite/statement.h | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 44d5921afd6d08..f1237548a33467 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -117,7 +117,7 @@ cursor_dealloc(pysqlite_Cursor *self) PyObject_ClearWeakRefs((PyObject*)self); } if (self->statement) { - (void)pysqlite_statement_reset(self->statement); + pysqlite_statement_reset(self->statement); } tp->tp_clear((PyObject *)self); tp->tp_free(self); @@ -565,7 +565,7 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation break; } - (void)pysqlite_statement_reset(self->statement); + pysqlite_statement_reset(self->statement); pysqlite_statement_mark_dirty(self->statement); pysqlite_statement_bind_parameters(state, self->statement, parameters); diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index 25cc7ba8d077e2..3016fe5bb45ce7 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -360,11 +360,12 @@ pysqlite_statement_bind_parameters(pysqlite_state *state, } } -int pysqlite_statement_reset(pysqlite_Statement* self) +void +pysqlite_statement_reset(pysqlite_Statement *self) { sqlite3_stmt *stmt = self->st; if (stmt == NULL || self->in_use == 0) { - return SQLITE_OK; + return; } #if SQLITE_VERSION_NUMBER >= 3020000 @@ -372,7 +373,7 @@ int pysqlite_statement_reset(pysqlite_Statement* self) * called at least once). Third parameter is non-zero in order to reset the * run count. */ if (sqlite3_stmt_status(stmt, SQLITE_STMTSTATUS_RUN, 1) == 0) { - return SQLITE_OK; + return; } #endif @@ -384,7 +385,6 @@ int pysqlite_statement_reset(pysqlite_Statement* self) if (rc == SQLITE_OK) { self->in_use = 0; } - return rc; } void pysqlite_statement_mark_dirty(pysqlite_Statement* self) diff --git a/Modules/_sqlite/statement.h b/Modules/_sqlite/statement.h index b901c43c479ae2..cce81ed910de04 100644 --- a/Modules/_sqlite/statement.h +++ b/Modules/_sqlite/statement.h @@ -44,7 +44,7 @@ void pysqlite_statement_bind_parameters(pysqlite_state *state, pysqlite_Statement *self, PyObject *parameters); -int pysqlite_statement_reset(pysqlite_Statement* self); +void pysqlite_statement_reset(pysqlite_Statement *self); void pysqlite_statement_mark_dirty(pysqlite_Statement* self); int pysqlite_statement_setup_types(PyObject *module); From 4ad7e99b38b7e32a15be39324181f740e7672f11 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 6 Sep 2021 23:54:10 +0200 Subject: [PATCH 07/10] Make sure we reset statements both at cursor dealloc and when the current cursor statement is replaced. This is needed because SELECT queries will lock tables until they are either done or reset. --- Modules/_sqlite/cursor.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index f1237548a33467..d54aeb4a129073 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -117,7 +117,12 @@ cursor_dealloc(pysqlite_Cursor *self) PyObject_ClearWeakRefs((PyObject*)self); } if (self->statement) { - pysqlite_statement_reset(self->statement); + /* A SELECT query will lock the affected database table(s), so we need + * to reset the statement to unlock the database before disappearing */ + sqlite3_stmt *stmt = self->statement->st; + if (sqlite3_stmt_readonly(stmt)) { + pysqlite_statement_reset(self->statement); + } } tp->tp_clear((PyObject *)self); tp->tp_free(self); @@ -526,6 +531,16 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation Py_SETREF(self->description, Py_None); self->rowcount = 0L; + if (self->statement) { + /* A SELECT query will lock the affected database table(s), so we need + * to reset the statement to unlock the database before switching + * statements */ + sqlite3_stmt *stmt = self->statement->st; + if (sqlite3_stmt_readonly(stmt)) { + pysqlite_statement_reset(self->statement); + } + } + PyObject *stmt = get_statement_from_cache(self, operation); Py_XSETREF(self->statement, (pysqlite_Statement *)stmt); if (!self->statement) { From 1bbb31f04f1264b6176a58ad6cba1e3061ae9deb Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 6 Sep 2021 23:59:26 +0200 Subject: [PATCH 08/10] Harden test_type_map_usage() Don't rely on the GC dealloc'ing the implicitly created cursor executing the first select query; use one cursor, so we know that the select query won't block the subsequent delete statement. --- Lib/sqlite3/test/regression.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py index ddf36e71819445..faf7e6352a6f3a 100644 --- a/Lib/sqlite3/test/regression.py +++ b/Lib/sqlite3/test/regression.py @@ -124,13 +124,14 @@ def test_type_map_usage(self): """ SELECT = "select * from foo" con = sqlite.connect(":memory:",detect_types=sqlite.PARSE_DECLTYPES) - con.execute("create table foo(bar timestamp)") - con.execute("insert into foo(bar) values (?)", (datetime.datetime.now(),)) - con.execute(SELECT) - con.execute("drop table foo") - con.execute("create table foo(bar integer)") - con.execute("insert into foo(bar) values (5)") - con.execute(SELECT) + cur = con.cursor() + cur.execute("create table foo(bar timestamp)") + cur.execute("insert into foo(bar) values (?)", (datetime.datetime.now(),)) + cur.execute(SELECT) + cur.execute("drop table foo") + cur.execute("create table foo(bar integer)") + cur.execute("insert into foo(bar) values (5)") + cur.execute(SELECT) def test_bind_mutating_list(self): # Issue41662: Crash when mutate a list of parameters during iteration. From 73e1ddb20d635faf4e627c9fc95de94e5d1ec8ac Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 7 Sep 2021 00:32:54 +0200 Subject: [PATCH 09/10] Add regression tests for stmt exit from cursor --- Lib/sqlite3/test/regression.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/Lib/sqlite3/test/regression.py b/Lib/sqlite3/test/regression.py index faf7e6352a6f3a..3a71def0a15e36 100644 --- a/Lib/sqlite3/test/regression.py +++ b/Lib/sqlite3/test/regression.py @@ -436,6 +436,26 @@ def test_return_empty_bytestring(self): val = cur.fetchone()[0] self.assertEqual(val, b'') + def test_table_lock_cursor_replace_stmt(self): + con = sqlite.connect(":memory:") + cur = con.cursor() + cur.execute("create table t(t)") + cur.executemany("insert into t values(?)", ((v,) for v in range(5))) + con.commit() + cur.execute("select t from t") + cur.execute("drop table t") + con.commit() + + def test_table_lock_cursor_dealloc(self): + con = sqlite.connect(":memory:") + con.execute("create table t(t)") + con.executemany("insert into t values(?)", ((v,) for v in range(5))) + con.commit() + cur = con.execute("select t from t") + del cur + con.execute("drop table t") + con.commit() + def suite(): tests = [ From c84311e8a3655b4037459db2925c2f03861f074a Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 21 Sep 2021 01:34:52 +0200 Subject: [PATCH 10/10] Test that datamodifying select queries are marked readonly https://github.com/python/cpython/pull/27844#discussion_r712574524 --- Lib/sqlite3/test/test_regression.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Lib/sqlite3/test/test_regression.py b/Lib/sqlite3/test/test_regression.py index 19ab970123f45a..d41f11814133a5 100644 --- a/Lib/sqlite3/test/test_regression.py +++ b/Lib/sqlite3/test/test_regression.py @@ -456,6 +456,20 @@ def test_table_lock_cursor_dealloc(self): con.execute("drop table t") con.commit() + def test_table_lock_cursor_non_readonly_select(self): + con = sqlite.connect(":memory:") + con.execute("create table t(t)") + con.executemany("insert into t values(?)", ((v,) for v in range(5))) + con.commit() + def dup(v): + con.execute("insert into t values(?)", (v,)) + return + con.create_function("dup", 1, dup) + cur = con.execute("select dup(t) from t") + del cur + con.execute("drop table t") + con.commit() + if __name__ == "__main__": unittest.main()