From ab994ed8b97e1b0dac151ec827c857f5e7277565 Mon Sep 17 00:00:00 2001 From: Berker Peksag Date: Sun, 11 Sep 2016 12:57:15 +0300 Subject: [PATCH] Issue #10740: sqlite3 no longer implicitly commit an open transaction before DDL statements This commit contains the following commits from ghaering/pysqlite: * https://github.com/ghaering/pysqlite/commit/f254c534948c41c0ceb8cbabf0d4a2f547754739 * https://github.com/ghaering/pysqlite/commit/796b3afe38cfdac5d7d5ec260826b0a596554631 * https://github.com/ghaering/pysqlite/commit/cae87ee68613697a5f4947b4a0941f59a28da1b6 * https://github.com/ghaering/pysqlite/commit/3567b31bb5e5b226ba006213a9c69dde3f155faf With the following additions: * Fixed a refcount error * Fixed a compiler warning * Made the string comparison a little more robust * Added a whatsnew entry --- Doc/library/sqlite3.rst | 7 +- Doc/whatsnew/3.6.rst | 3 + Lib/sqlite3/test/transactions.py | 36 +++++++-- Misc/NEWS | 3 + Modules/_sqlite/cursor.c | 125 +++++++------------------------ Modules/_sqlite/cursor.h | 6 -- Modules/_sqlite/statement.c | 18 +++++ Modules/_sqlite/statement.h | 1 + 8 files changed, 83 insertions(+), 116 deletions(-) diff --git a/Doc/library/sqlite3.rst b/Doc/library/sqlite3.rst index d5d6e6b7004..76693bd43c5 100644 --- a/Doc/library/sqlite3.rst +++ b/Doc/library/sqlite3.rst @@ -925,9 +925,7 @@ Controlling Transactions By default, the :mod:`sqlite3` module opens transactions implicitly before a Data Modification Language (DML) statement (i.e. -``INSERT``/``UPDATE``/``DELETE``/``REPLACE``), and commits transactions -implicitly before a non-DML, non-query statement (i. e. -anything other than ``SELECT`` or the aforementioned). +``INSERT``/``UPDATE``/``DELETE``/``REPLACE``). So if you are within a transaction and issue a command like ``CREATE TABLE ...``, ``VACUUM``, ``PRAGMA``, the :mod:`sqlite3` module will commit implicitly @@ -947,6 +945,9 @@ Otherwise leave it at its default, which will result in a plain "BEGIN" statement, or set it to one of SQLite's supported isolation levels: "DEFERRED", "IMMEDIATE" or "EXCLUSIVE". +.. versionchanged:: 3.6 + :mod:`sqlite3` used to implicitly commit an open transaction before DDL + statements. This is no longer the case. Using :mod:`sqlite3` efficiently diff --git a/Doc/whatsnew/3.6.rst b/Doc/whatsnew/3.6.rst index 8752b83e638..a3f216f507e 100644 --- a/Doc/whatsnew/3.6.rst +++ b/Doc/whatsnew/3.6.rst @@ -1195,6 +1195,9 @@ Changes in 'python' Command Behavior Changes in the Python API ------------------------- +* :mod:`sqlite3` no longer implicitly commit an open transaction before DDL + statements. + * On Linux, :func:`os.urandom` now blocks until the system urandom entropy pool is initialized to increase the security. diff --git a/Lib/sqlite3/test/transactions.py b/Lib/sqlite3/test/transactions.py index a25360a7d82..45f1b04c696 100644 --- a/Lib/sqlite3/test/transactions.py +++ b/Lib/sqlite3/test/transactions.py @@ -52,13 +52,13 @@ class TransactionTests(unittest.TestCase): except OSError: pass - def CheckDMLdoesAutoCommitBefore(self): + def CheckDMLDoesNotAutoCommitBefore(self): self.cur1.execute("create table test(i)") self.cur1.execute("insert into test(i) values (5)") self.cur1.execute("create table test2(j)") self.cur2.execute("select i from test") res = self.cur2.fetchall() - self.assertEqual(len(res), 1) + self.assertEqual(len(res), 0) def CheckInsertStartsTransaction(self): self.cur1.execute("create table test(i)") @@ -153,11 +153,6 @@ class SpecialCommandTests(unittest.TestCase): self.con = sqlite.connect(":memory:") self.cur = self.con.cursor() - def CheckVacuum(self): - self.cur.execute("create table test(i)") - self.cur.execute("insert into test(i) values (5)") - self.cur.execute("vacuum") - def CheckDropTable(self): self.cur.execute("create table test(i)") self.cur.execute("insert into test(i) values (5)") @@ -172,10 +167,35 @@ class SpecialCommandTests(unittest.TestCase): self.cur.close() self.con.close() +class TransactionalDDL(unittest.TestCase): + def setUp(self): + self.con = sqlite.connect(":memory:") + + def CheckDdlDoesNotAutostartTransaction(self): + # For backwards compatibility reasons, DDL statements should not + # implicitly start a transaction. + self.con.execute("create table test(i)") + self.con.rollback() + result = self.con.execute("select * from test").fetchall() + self.assertEqual(result, []) + + def CheckTransactionalDDL(self): + # You can achieve transactional DDL by issuing a BEGIN + # statement manually. + self.con.execute("begin") + self.con.execute("create table test(i)") + self.con.rollback() + with self.assertRaises(sqlite.OperationalError): + self.con.execute("select * from test") + + def tearDown(self): + self.con.close() + def suite(): default_suite = unittest.makeSuite(TransactionTests, "Check") special_command_suite = unittest.makeSuite(SpecialCommandTests, "Check") - return unittest.TestSuite((default_suite, special_command_suite)) + ddl_suite = unittest.makeSuite(TransactionalDDL, "Check") + return unittest.TestSuite((default_suite, special_command_suite, ddl_suite)) def test(): runner = unittest.TextTestRunner() diff --git a/Misc/NEWS b/Misc/NEWS index fe5fab147de..898e26c6bd8 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -143,6 +143,9 @@ Core and Builtins Library ------- +- Issue #10740: sqlite3 no longer implicitly commit an open transaction + before DDL statements. + - Issue #22493: Inline flags now should be used only at the start of the regular expression. Deprecation warning is emitted if uses them in the middle of the regular expression. diff --git a/Modules/_sqlite/cursor.c b/Modules/_sqlite/cursor.c index e2c7a3469a3..020f93107e0 100644 --- a/Modules/_sqlite/cursor.c +++ b/Modules/_sqlite/cursor.c @@ -29,44 +29,6 @@ PyObject* pysqlite_cursor_iternext(pysqlite_Cursor* self); static const char errmsg_fetch_across_rollback[] = "Cursor needed to be reset because of commit/rollback and can no longer be fetched from."; -static pysqlite_StatementKind detect_statement_type(const char* statement) -{ - char buf[20]; - const char* src; - char* dst; - - src = statement; - /* skip over whitepace */ - while (*src == '\r' || *src == '\n' || *src == ' ' || *src == '\t') { - src++; - } - - if (*src == 0) - return STATEMENT_INVALID; - - dst = buf; - *dst = 0; - while (Py_ISALPHA(*src) && (dst - buf) < ((Py_ssize_t)sizeof(buf) - 2)) { - *dst++ = Py_TOLOWER(*src++); - } - - *dst = 0; - - if (!strcmp(buf, "select")) { - return STATEMENT_SELECT; - } else if (!strcmp(buf, "insert")) { - return STATEMENT_INSERT; - } else if (!strcmp(buf, "update")) { - return STATEMENT_UPDATE; - } else if (!strcmp(buf, "delete")) { - return STATEMENT_DELETE; - } else if (!strcmp(buf, "replace")) { - return STATEMENT_REPLACE; - } else { - return STATEMENT_OTHER; - } -} - static int pysqlite_cursor_init(pysqlite_Cursor* self, PyObject* args, PyObject* kwargs) { pysqlite_Connection* connection; @@ -427,9 +389,9 @@ PyObject* _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* PyObject* func_args; PyObject* result; int numcols; - int statement_type; PyObject* descriptor; PyObject* second_argument = NULL; + sqlite_int64 lastrowid; if (!check_cursor(self)) { goto error; @@ -510,7 +472,7 @@ PyObject* _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* /* reset description and rowcount */ Py_INCREF(Py_None); Py_SETREF(self->description, Py_None); - self->rowcount = -1L; + self->rowcount = 0L; func_args = PyTuple_New(1); if (!func_args) { @@ -549,43 +511,19 @@ PyObject* _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* pysqlite_statement_reset(self->statement); pysqlite_statement_mark_dirty(self->statement); - statement_type = detect_statement_type(operation_cstr); - if (self->connection->begin_statement) { - switch (statement_type) { - case STATEMENT_UPDATE: - case STATEMENT_DELETE: - case STATEMENT_INSERT: - case STATEMENT_REPLACE: - if (!self->connection->inTransaction) { - result = _pysqlite_connection_begin(self->connection); - if (!result) { - goto error; - } - Py_DECREF(result); - } - break; - case STATEMENT_OTHER: - /* it's a DDL statement or something similar - - we better COMMIT first so it works for all cases */ - if (self->connection->inTransaction) { - result = pysqlite_connection_commit(self->connection, NULL); - if (!result) { - goto error; - } - Py_DECREF(result); - } - break; - case STATEMENT_SELECT: - if (multiple) { - PyErr_SetString(pysqlite_ProgrammingError, - "You cannot execute SELECT statements in executemany()."); - goto error; - } - break; + /* For backwards compatibility reasons, do not start a transaction if a + DDL statement is encountered. If anybody wants transactional DDL, + they can issue a BEGIN statement manually. */ + if (self->connection->begin_statement && !sqlite3_stmt_readonly(self->statement->st) && !self->statement->is_ddl) { + if (sqlite3_get_autocommit(self->connection->db)) { + result = _pysqlite_connection_begin(self->connection); + if (!result) { + goto error; + } + Py_DECREF(result); } } - while (1) { parameters = PyIter_Next(parameters_iter); if (!parameters) { @@ -671,6 +609,20 @@ PyObject* _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* } } + if (!sqlite3_stmt_readonly(self->statement->st)) { + self->rowcount += (long)sqlite3_changes(self->connection->db); + } else { + self->rowcount= -1L; + } + + if (!multiple) { + Py_DECREF(self->lastrowid); + Py_BEGIN_ALLOW_THREADS + lastrowid = sqlite3_last_insert_rowid(self->connection->db); + Py_END_ALLOW_THREADS + self->lastrowid = _pysqlite_long_from_int64(lastrowid); + } + if (rc == SQLITE_ROW) { if (multiple) { PyErr_SetString(pysqlite_ProgrammingError, "executemany() can only execute DML statements."); @@ -685,31 +637,6 @@ PyObject* _pysqlite_query_execute(pysqlite_Cursor* self, int multiple, PyObject* Py_CLEAR(self->statement); } - switch (statement_type) { - case STATEMENT_UPDATE: - case STATEMENT_DELETE: - case STATEMENT_INSERT: - case STATEMENT_REPLACE: - if (self->rowcount == -1L) { - self->rowcount = 0L; - } - self->rowcount += (long)sqlite3_changes(self->connection->db); - } - - Py_DECREF(self->lastrowid); - if (!multiple && - /* REPLACE is an alias for INSERT OR REPLACE */ - (statement_type == STATEMENT_INSERT || statement_type == STATEMENT_REPLACE)) { - sqlite_int64 lastrowid; - Py_BEGIN_ALLOW_THREADS - lastrowid = sqlite3_last_insert_rowid(self->connection->db); - Py_END_ALLOW_THREADS - self->lastrowid = _pysqlite_long_from_int64(lastrowid); - } else { - Py_INCREF(Py_None); - self->lastrowid = Py_None; - } - if (multiple) { pysqlite_statement_reset(self->statement); } diff --git a/Modules/_sqlite/cursor.h b/Modules/_sqlite/cursor.h index 118ba388a41..28bbd5f9117 100644 --- a/Modules/_sqlite/cursor.h +++ b/Modules/_sqlite/cursor.h @@ -51,12 +51,6 @@ typedef struct PyObject* in_weakreflist; /* List of weak references */ } pysqlite_Cursor; -typedef enum { - STATEMENT_INVALID, STATEMENT_INSERT, STATEMENT_DELETE, - STATEMENT_UPDATE, STATEMENT_REPLACE, STATEMENT_SELECT, - STATEMENT_OTHER -} pysqlite_StatementKind; - extern PyTypeObject pysqlite_CursorType; PyObject* pysqlite_cursor_execute(pysqlite_Cursor* self, PyObject* args); diff --git a/Modules/_sqlite/statement.c b/Modules/_sqlite/statement.c index e87063341db..7b980135ed0 100644 --- a/Modules/_sqlite/statement.c +++ b/Modules/_sqlite/statement.c @@ -54,6 +54,7 @@ int pysqlite_statement_create(pysqlite_Statement* self, pysqlite_Connection* con int rc; const char* sql_cstr; Py_ssize_t sql_cstr_len; + const char* p; self->st = NULL; self->in_use = 0; @@ -72,6 +73,23 @@ int pysqlite_statement_create(pysqlite_Statement* self, pysqlite_Connection* con Py_INCREF(sql); self->sql = sql; + /* determine if the statement is a DDL statement */ + self->is_ddl = 0; + for (p = sql_cstr; *p != 0; p++) { + switch (*p) { + case ' ': + case '\r': + case '\n': + case '\t': + continue; + } + + self->is_ddl = (PyOS_strnicmp(p, "create ", 7) == 0) + || (PyOS_strnicmp(p, "drop ", 5) == 0) + || (PyOS_strnicmp(p, "reindex ", 8) == 0); + break; + } + Py_BEGIN_ALLOW_THREADS rc = sqlite3_prepare(connection->db, sql_cstr, diff --git a/Modules/_sqlite/statement.h b/Modules/_sqlite/statement.h index 4681443e760..6eef16857f7 100644 --- a/Modules/_sqlite/statement.h +++ b/Modules/_sqlite/statement.h @@ -38,6 +38,7 @@ typedef struct sqlite3_stmt* st; PyObject* sql; int in_use; + int is_ddl; PyObject* in_weakreflist; /* List of weak references */ } pysqlite_Statement;