From 5436db7e3d9f1de8cf1e1049a7ff7e7db2061080 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sun, 5 Jun 2022 09:10:10 +0200 Subject: [PATCH 1/8] Try lazy rowcount approach Does not work bca. executemany, but it solves the repro --- Modules/_sqlite/cursor.c | 28 +++++++++++++++++----------- Modules/_sqlite/cursor.h | 1 - 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index c58def5f0362f1..a559b3e66e039e 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -107,7 +107,6 @@ pysqlite_cursor_init_impl(pysqlite_Cursor *self, self->arraysize = 1; self->closed = 0; - self->rowcount = -1L; Py_INCREF(Py_None); Py_XSETREF(self->row_factory, Py_None); @@ -835,10 +834,9 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation stmt_reset(self->statement); } - /* reset description and rowcount */ + /* reset description */ Py_INCREF(Py_None); Py_SETREF(self->description, Py_None); - self->rowcount = 0L; if (self->statement) { (void)stmt_reset(self->statement); @@ -944,12 +942,6 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation } } - if (self->statement->is_dml) { - self->rowcount += (long)sqlite3_changes(self->connection->db); - } else { - self->rowcount= -1L; - } - if (rc == SQLITE_DONE && !multiple) { stmt_reset(self->statement); Py_CLEAR(self->statement); @@ -980,7 +972,6 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation self->locked = 0; if (PyErr_Occurred()) { - self->rowcount = -1L; return NULL; } else { return Py_NewRef((PyObject *)self); @@ -1321,13 +1312,27 @@ static PyMethodDef cursor_methods[] = { {NULL, NULL} }; +static PyObject * +get_rowcount(pysqlite_Cursor *self, void *Py_UNUSED(unused)) +{ + if (!check_cursor(self)) { + return NULL; + } + int changes = sqlite3_changes(self->connection->db); + return PyLong_FromLong(changes); +} + +static PyGetSetDef cursor_getset[] = { + {"rowcount", (getter)get_rowcount, (setter)NULL}, + {NULL}, +}; + static struct PyMemberDef cursor_members[] = { {"connection", T_OBJECT, offsetof(pysqlite_Cursor, connection), READONLY}, {"description", T_OBJECT, offsetof(pysqlite_Cursor, description), READONLY}, {"arraysize", T_INT, offsetof(pysqlite_Cursor, arraysize), 0}, {"lastrowid", T_OBJECT, offsetof(pysqlite_Cursor, lastrowid), READONLY}, - {"rowcount", T_LONG, offsetof(pysqlite_Cursor, rowcount), READONLY}, {"row_factory", T_OBJECT, offsetof(pysqlite_Cursor, row_factory), 0}, {"__weaklistoffset__", T_PYSSIZET, offsetof(pysqlite_Cursor, in_weakreflist), READONLY}, {NULL} @@ -1346,6 +1351,7 @@ static PyType_Slot cursor_slots[] = { {Py_tp_init, pysqlite_cursor_init}, {Py_tp_traverse, cursor_traverse}, {Py_tp_clear, cursor_clear}, + {Py_tp_getset, cursor_getset}, {0, NULL}, }; diff --git a/Modules/_sqlite/cursor.h b/Modules/_sqlite/cursor.h index 0bcdddc3e29595..04dc2427a83adc 100644 --- a/Modules/_sqlite/cursor.h +++ b/Modules/_sqlite/cursor.h @@ -38,7 +38,6 @@ typedef struct PyObject* row_cast_map; int arraysize; PyObject* lastrowid; - long rowcount; PyObject* row_factory; pysqlite_Statement* statement; int closed; From 84891c6fffc6f44d92bc1f9d3cbad00cf3593fe1 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sun, 5 Jun 2022 11:13:14 +0200 Subject: [PATCH 2/8] Calculate rowcount using sqlite3_total_changes() diff --- Modules/_sqlite/cursor.c | 45 ++++++++++++++++++++++++++-------------- Modules/_sqlite/cursor.h | 1 + 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index a559b3e66e039e..17271d4d8be2bc 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -107,6 +107,7 @@ pysqlite_cursor_init_impl(pysqlite_Cursor *self, self->arraysize = 1; self->closed = 0; + self->rowcount = -1L; Py_INCREF(Py_None); Py_XSETREF(self->row_factory, Py_None); @@ -855,7 +856,7 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation goto error; } - if (self->statement->in_use) { + if (sqlite3_stmt_readonly(self->statement->st)) { Py_SETREF(self->statement, pysqlite_statement_create(self->connection, operation)); if (self->statement == NULL) { @@ -866,6 +867,14 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation stmt_reset(self->statement); stmt_mark_dirty(self->statement); + // Save current row count + if (sqlite3_stmt_readonly(self->statement->st)) { + self->rowcount = -1L; + } + else { + self->rowcount = (long)sqlite3_total_changes(self->connection->db); + } + /* We start a transaction implicitly before a DML statement. SELECT is the only exception. See #9924. */ if (self->connection->isolation_level @@ -972,6 +981,7 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation self->locked = 0; if (PyErr_Occurred()) { + self->rowcount = -1L; return NULL; } else { return Py_NewRef((PyObject *)self); @@ -1299,6 +1309,19 @@ pysqlite_cursor_close_impl(pysqlite_Cursor *self) Py_RETURN_NONE; } +static PyObject * +get_rowcount(pysqlite_Cursor *self, void *Py_UNUSED(closure)) +{ + if (!check_cursor(self)) { + return -1; + } + if (self->rowcount != -1L) { + long changes = (long)sqlite3_total_changes(self->connection->db); + return PyLong_FromLong(changes - self->rowcount); + } + return PyLong_FromLong(-1L); +} + static PyMethodDef cursor_methods[] = { PYSQLITE_CURSOR_CLOSE_METHODDEF PYSQLITE_CURSOR_EXECUTEMANY_METHODDEF @@ -1312,21 +1335,6 @@ static PyMethodDef cursor_methods[] = { {NULL, NULL} }; -static PyObject * -get_rowcount(pysqlite_Cursor *self, void *Py_UNUSED(unused)) -{ - if (!check_cursor(self)) { - return NULL; - } - int changes = sqlite3_changes(self->connection->db); - return PyLong_FromLong(changes); -} - -static PyGetSetDef cursor_getset[] = { - {"rowcount", (getter)get_rowcount, (setter)NULL}, - {NULL}, -}; - static struct PyMemberDef cursor_members[] = { {"connection", T_OBJECT, offsetof(pysqlite_Cursor, connection), READONLY}, @@ -1338,6 +1346,11 @@ static struct PyMemberDef cursor_members[] = {NULL} }; +static PyGetSetDef cursor_getset[] = { + {"rowcount", (getter)get_rowcount, (setter)NULL}, + {NULL}, +}; + static const char cursor_doc[] = PyDoc_STR("SQLite database cursor class."); diff --git a/Modules/_sqlite/cursor.h b/Modules/_sqlite/cursor.h index 04dc2427a83adc..d0757db41a80a5 100644 --- a/Modules/_sqlite/cursor.h +++ b/Modules/_sqlite/cursor.h @@ -38,6 +38,7 @@ typedef struct PyObject* row_cast_map; int arraysize; PyObject* lastrowid; + long rowcount; // saved row count PyObject* row_factory; pysqlite_Statement* statement; int closed; From a5fca9d2a6114c279309f5b23e69aa354d8552cc Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sun, 5 Jun 2022 22:22:47 +0200 Subject: [PATCH 3/8] Add NEWS --- .../next/Library/2022-06-05-22-22-42.gh-issue-93421.43UO_8.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2022-06-05-22-22-42.gh-issue-93421.43UO_8.rst diff --git a/Misc/NEWS.d/next/Library/2022-06-05-22-22-42.gh-issue-93421.43UO_8.rst b/Misc/NEWS.d/next/Library/2022-06-05-22-22-42.gh-issue-93421.43UO_8.rst new file mode 100644 index 00000000000000..085ea94c03c64e --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-06-05-22-22-42.gh-issue-93421.43UO_8.rst @@ -0,0 +1,2 @@ +Fix :data:`sqlite3.Cursor.rowcount` for ``UPDATE ... RETURNING``` SQL +queries. Patch by Erlend E. Aasland. From 58199023d3d828b937294c0d75656cbf5da3c03a Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sun, 5 Jun 2022 22:56:25 +0200 Subject: [PATCH 4/8] Fix return value --- Modules/_sqlite/cursor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 17271d4d8be2bc..7852f38f6ce2ed 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -1313,7 +1313,7 @@ static PyObject * get_rowcount(pysqlite_Cursor *self, void *Py_UNUSED(closure)) { if (!check_cursor(self)) { - return -1; + return NULL; } if (self->rowcount != -1L) { long changes = (long)sqlite3_total_changes(self->connection->db); From 5165a3a46f828efb1bcfa36a48d73ab8d79fc0da Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sun, 5 Jun 2022 23:53:47 +0200 Subject: [PATCH 5/8] Add tests --- Lib/test/test_sqlite3/test_dbapi.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 1fa02db3b3af41..54b55cc6999548 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -887,6 +887,21 @@ def test_rowcount_executemany(self): self.cu.executemany("insert into test(name) values (?)", [(1,), (2,), (3,)]) self.assertEqual(self.cu.rowcount, 3) + def test_rowcount_prefixed_with_comment(self): + # gh-79579: rowcount is updated even if query is prefixed with comments + self.cu.execute("/* foo */ insert into test(name) values (?)", ('foo',)) + self.assertEqual(self.cu.rowcount, 1) + self.cu.execute("/* bar */ update test set name='bar' where name='foo'") + self.assertEqual(self.cu.rowcount, 2) + + @unittest.skipIf(sqlite.sqlite_version_info < (3, 35, 0), + "Requires SQLite 3.35.0 or newer") + def test_rowcount_update_returning(self): + # gh-93421: rowcount is updated correctly for UPDATE...RETURNING queries + self.cu.execute("update test set name='bar' where name='foo' returning 1") + self.assertEqual(self.cu.fetchone()[0], 1) + self.assertEqual(self.cu.rowcount, 1) + def test_total_changes(self): self.cu.execute("insert into test(name) values ('foo')") self.cu.execute("insert into test(name) values ('foo')") From 1da6390fe513b69f0edb1fc92fe6a30e52cc589e Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 6 Jun 2022 00:19:24 +0200 Subject: [PATCH 6/8] Remove spurious change --- Modules/_sqlite/cursor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 7852f38f6ce2ed..49630393b783f3 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -856,7 +856,7 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation goto error; } - if (sqlite3_stmt_readonly(self->statement->st)) { + if (self->statement->in_use) { Py_SETREF(self->statement, pysqlite_statement_create(self->connection, operation)); if (self->statement == NULL) { From c4f6926b9719f0eeba297390ae809d37f866d02e Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 6 Jun 2022 22:08:37 +0200 Subject: [PATCH 7/8] Revert sqlite3_stmt_readonly change --- Lib/test/test_sqlite3/test_dbapi.py | 7 ------- Modules/_sqlite/cursor.c | 24 ++++++++++++------------ 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 54b55cc6999548..fb14c0bc71d99e 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -887,13 +887,6 @@ def test_rowcount_executemany(self): self.cu.executemany("insert into test(name) values (?)", [(1,), (2,), (3,)]) self.assertEqual(self.cu.rowcount, 3) - def test_rowcount_prefixed_with_comment(self): - # gh-79579: rowcount is updated even if query is prefixed with comments - self.cu.execute("/* foo */ insert into test(name) values (?)", ('foo',)) - self.assertEqual(self.cu.rowcount, 1) - self.cu.execute("/* bar */ update test set name='bar' where name='foo'") - self.assertEqual(self.cu.rowcount, 2) - @unittest.skipIf(sqlite.sqlite_version_info < (3, 35, 0), "Requires SQLite 3.35.0 or newer") def test_rowcount_update_returning(self): diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 49630393b783f3..296f20d609e3a1 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -867,14 +867,6 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation stmt_reset(self->statement); stmt_mark_dirty(self->statement); - // Save current row count - if (sqlite3_stmt_readonly(self->statement->st)) { - self->rowcount = -1L; - } - else { - self->rowcount = (long)sqlite3_total_changes(self->connection->db); - } - /* We start a transaction implicitly before a DML statement. SELECT is the only exception. See #9924. */ if (self->connection->isolation_level @@ -886,6 +878,14 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation } } + if (self->statement->is_dml) { + // Save current row count + self->rowcount = (long)sqlite3_total_changes(self->connection->db); + } + else { + self->rowcount = -1L; + } + while (1) { parameters = PyIter_Next(parameters_iter); if (!parameters) { @@ -1315,11 +1315,11 @@ get_rowcount(pysqlite_Cursor *self, void *Py_UNUSED(closure)) if (!check_cursor(self)) { return NULL; } - if (self->rowcount != -1L) { - long changes = (long)sqlite3_total_changes(self->connection->db); - return PyLong_FromLong(changes - self->rowcount); + if (self->rowcount == -1L) { + return PyLong_FromLong(-1L); } - return PyLong_FromLong(-1L); + long changes = (long)sqlite3_total_changes(self->connection->db); + return PyLong_FromLong(changes - self->rowcount); } static PyMethodDef cursor_methods[] = { From 20397c5c6c53e874f31f65b3435bd648256f9ebe Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 6 Jun 2022 22:36:39 +0200 Subject: [PATCH 8/8] Use sqlite3_total_changes64 if available --- Modules/_sqlite/cursor.c | 14 ++++++++++++-- Modules/_sqlite/cursor.h | 2 +- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index 296f20d609e3a1..115f0c83f095d9 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -776,6 +776,16 @@ stmt_mark_dirty(pysqlite_Statement *self) self->in_use = 1; } +static inline sqlite3_int64 +total_changes(sqlite3 *db) +{ +#if SQLITE_VERSION_NUMBER >= 3037000 + return sqlite3_total_changes64(db); +#else + return (sqlite3_int64)sqlite3_total_changes(db); +#endif +} + PyObject * _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation, PyObject* second_argument) { @@ -880,7 +890,7 @@ _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* operation if (self->statement->is_dml) { // Save current row count - self->rowcount = (long)sqlite3_total_changes(self->connection->db); + self->rowcount = total_changes(self->connection->db); } else { self->rowcount = -1L; @@ -1318,7 +1328,7 @@ get_rowcount(pysqlite_Cursor *self, void *Py_UNUSED(closure)) if (self->rowcount == -1L) { return PyLong_FromLong(-1L); } - long changes = (long)sqlite3_total_changes(self->connection->db); + sqlite3_int64 changes = total_changes(self->connection->db); return PyLong_FromLong(changes - self->rowcount); } diff --git a/Modules/_sqlite/cursor.h b/Modules/_sqlite/cursor.h index d0757db41a80a5..3ef1caf1a1edd9 100644 --- a/Modules/_sqlite/cursor.h +++ b/Modules/_sqlite/cursor.h @@ -38,7 +38,7 @@ typedef struct PyObject* row_cast_map; int arraysize; PyObject* lastrowid; - long rowcount; // saved row count + sqlite3_int64 rowcount; // Saved row count PyObject* row_factory; pysqlite_Statement* statement; int closed;