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

@@ -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

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.
 

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()

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.

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);
         }

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);

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,

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;