From 0efd40e5c280f018bbf6a0aa14afdecfc747d6cb Mon Sep 17 00:00:00 2001 From: NightjarNZ Date: Tue, 9 Oct 2018 22:22:53 +1300 Subject: [PATCH 1/5] FIX correct handwritten logic for transactions to use new API instead Code in the field alteration logic had a queries defiend as strings to begin and commit transactions involve with changing table or column names. This was causing fatal errors as BEGIN is not a valid keyword within a trasaction (see SQLite documentation excerpt below). A new api has been introduced to deal with transactions programmatically, and this module was updated to support this a few months ago. This is a tidy up of some missed portions - consuming this API which correctly uses SAVEPOINT when a nested transaction is required automatically. https://www.sqlite.org/lang_transaction.html Transactions created using BEGIN...COMMIT do not nest. For nested transactions, use the SAVEPOINT and RELEASE commands. --- code/SQLite3SchemaManager.php | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/code/SQLite3SchemaManager.php b/code/SQLite3SchemaManager.php index 91c47eb..a993dc7 100644 --- a/code/SQLite3SchemaManager.php +++ b/code/SQLite3SchemaManager.php @@ -275,21 +275,22 @@ class SQLite3SchemaManager extends DBSchemaManager } $queries = array( - "BEGIN TRANSACTION", "CREATE TABLE \"{$tableName}_alterfield_{$fieldName}\"(" . implode(',', $newColsSpec) . ")", "INSERT INTO \"{$tableName}_alterfield_{$fieldName}\" SELECT {$fieldNameList} FROM \"$tableName\"", "DROP TABLE \"$tableName\"", "ALTER TABLE \"{$tableName}_alterfield_{$fieldName}\" RENAME TO \"$tableName\"", - "COMMIT" ); // Remember original indexes $indexList = $this->indexList($tableName); // Then alter the table column - foreach ($queries as $query) { - $this->query($query.';'); - } + $database = $this->database; + $database->withTransaction(function() use ($database, $queries, $indexList) { + foreach ($queries as $query) { + $database->query($query . ';'); + } + }); // Recreate the indexes foreach ($indexList as $indexName => $indexSpec) { @@ -318,21 +319,22 @@ class SQLite3SchemaManager extends DBSchemaManager $oldColsStr = implode(',', $oldCols); $newColsSpecStr = implode(',', $newColsSpec); $queries = array( - "BEGIN TRANSACTION", "CREATE TABLE \"{$tableName}_renamefield_{$oldName}\" ({$newColsSpecStr})", "INSERT INTO \"{$tableName}_renamefield_{$oldName}\" SELECT {$oldColsStr} FROM \"$tableName\"", "DROP TABLE \"$tableName\"", "ALTER TABLE \"{$tableName}_renamefield_{$oldName}\" RENAME TO \"$tableName\"", - "COMMIT" ); // Remember original indexes $oldIndexList = $this->indexList($tableName); // Then alter the table column - foreach ($queries as $query) { - $this->query($query.';'); - } + $database = $this->database; + $database->withTransaction(function() use ($database, $queries) { + foreach ($queries as $query) { + $database->query($query . ';'); + } + }); // Recreate the indexes foreach ($oldIndexList as $indexName => $indexSpec) { From 62ef14f7118f6557637b428508569dc60cfdac6e Mon Sep 17 00:00:00 2001 From: NightjarNZ Date: Thu, 11 Oct 2018 00:02:12 +1300 Subject: [PATCH 2/5] FIX correct nesting level mismatches causing errors Transactions that used more than one level would cause errors if there were consecutive calls to start a transaction - because each query executed would clear the flag indicating that a transaction was already in progress. The comment for the logic to reset the nesting level on a query was indicating that DDL (data definition language) would not work within a transaction. This is untrue, and the module itself uses a transaction to alter table or field names. So this function has been converted to a no-op, deprecated to be removed in version 3 of this module. It is also no longer called upon each query. There have been some maintenance tidyups around this area also by abstracting the nested transaction flag manipulations into protected functions. --- code/SQLite3Database.php | 61 ++++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 21 deletions(-) diff --git a/code/SQLite3Database.php b/code/SQLite3Database.php index 006bc54..a85a6a3 100644 --- a/code/SQLite3Database.php +++ b/code/SQLite3Database.php @@ -470,12 +470,12 @@ class SQLite3Database extends Database public function transactionStart($transaction_mode = false, $session_characteristics = false) { - if ($this->transactionNesting > 0) { - $this->transactionSavepoint('NESTEDTRANSACTION' . $this->transactionNesting); + if ($this->transactionDepth()) { + $this->transactionSavepoint($this->getTransactionSavepointName()); } else { $this->query('BEGIN'); } - ++$this->transactionNesting; + $this->transactionDepthIncrease(); } public function transactionSavepoint($savepoint) @@ -483,6 +483,18 @@ class SQLite3Database extends Database $this->query("SAVEPOINT \"$savepoint\""); } + /** + * Fetch the name of the current savepoint + * {@see transactionDepth} should be greater than zero + * or the name will be invalid (because there are none). + * + * @return string + */ + protected function getTransactionSavepointName() + { + return 'NESTEDTRANSACTION' . $this->transactionDepth(); + } + public function transactionRollback($savepoint = false) { // Named transaction @@ -492,13 +504,13 @@ class SQLite3Database extends Database } // Fail if transaction isn't available - if (!$this->transactionNesting) { + if (!$this->transactionDepth()) { return false; } - --$this->transactionNesting; - if ($this->transactionNesting > 0) { - $this->transactionRollback('NESTEDTRANSACTION' . $this->transactionNesting); + $this->transactionDepthDecrease(); + if ($this->transactionDepth()) { + $this->transactionRollback($this->getTransactionSavepointName()); } else { $this->query('ROLLBACK;'); } @@ -513,17 +525,30 @@ class SQLite3Database extends Database public function transactionEnd($chain = false) { // Fail if transaction isn't available - if (!$this->transactionNesting) { + if (!$this->transactionDepth()) { return false; } - --$this->transactionNesting; - if ($this->transactionNesting <= 0) { - $this->transactionNesting = 0; - $this->query('COMMIT;'); - } + $this->query('COMMIT;'); + $this->transactionDepthDecrease(); return true; } + /** + * Increase the nested transaction level by one + */ + protected function transactionDepthIncrease() + { + ++$this->transactionNesting; + } + + /** + * Decrease the nested transaction level by one + */ + protected function transactionDepthDecrease() + { + --$this->transactionNesting; + } + /** * In error condition, set transactionNesting to zero */ @@ -534,28 +559,22 @@ class SQLite3Database extends Database public function query($sql, $errorLevel = E_USER_ERROR) { - $this->inspectQuery($sql); return parent::query($sql, $errorLevel); } public function preparedQuery($sql, $parameters, $errorLevel = E_USER_ERROR) { - $this->inspectQuery($sql); return parent::preparedQuery($sql, $parameters, $errorLevel); } /** * Inspect a SQL query prior to execution - * + * @deprecated 2..3 * @param string $sql */ protected function inspectQuery($sql) { - // Any DDL discards transactions. - $isDDL = $this->getConnector()->isQueryDDL($sql); - if ($isDDL) { - $this->resetTransactionNesting(); - } + // no-op } public function clearTable($table) From 0fa6b0fde778a1c68c74abbef80cd2e157ef74f4 Mon Sep 17 00:00:00 2001 From: NightjarNZ Date: Thu, 11 Oct 2018 21:51:32 +1300 Subject: [PATCH 3/5] FIX transaction depth related errors with invalid savepoint names The logic for cancelling a savepoint was incorrect, as the behaviour the logic was modelled on was for a different RDBMS - where a COMMIT would always close the most recently opened transaction. SQLite on contrast will commit the entire transaction, not just the most recent savepoint marker until current execution point. The correct manner to deal with a 'partial' commit is to call RELEASE . This revealed an error in the savepoint logic, in that if someone had supplied a savepoint name instead of relying on generated ones, the rollback command did not factor for this and always assumed generated savepoint names - again causing error. For this reason a new class member field has been introduced to track savepoint names in a stack fashion. --- code/SQLite3Database.php | 66 ++++++++++++++++++++++++++++++++-------- 1 file changed, 54 insertions(+), 12 deletions(-) diff --git a/code/SQLite3Database.php b/code/SQLite3Database.php index a85a6a3..07942ca 100644 --- a/code/SQLite3Database.php +++ b/code/SQLite3Database.php @@ -65,6 +65,11 @@ class SQLite3Database extends Database */ protected $transactionNesting = 0; + /** + * @var array + */ + protected $transactionSavepoints = []; + /** * List of default pragma values * @@ -471,28 +476,27 @@ class SQLite3Database extends Database public function transactionStart($transaction_mode = false, $session_characteristics = false) { if ($this->transactionDepth()) { - $this->transactionSavepoint($this->getTransactionSavepointName()); + $this->transactionSavepoint('NESTEDTRANSACTION' . $this->transactionDepth()); } else { $this->query('BEGIN'); + $this->transactionDepthIncrease(); } - $this->transactionDepthIncrease(); } public function transactionSavepoint($savepoint) { $this->query("SAVEPOINT \"$savepoint\""); + $this->transactionDepthIncrease($savepoint); } /** - * Fetch the name of the current savepoint - * {@see transactionDepth} should be greater than zero - * or the name will be invalid (because there are none). + * Fetch the name of the most recent savepoint * * @return string */ protected function getTransactionSavepointName() { - return 'NESTEDTRANSACTION' . $this->transactionDepth(); + return end($this->transactionSavepoints); } public function transactionRollback($savepoint = false) @@ -500,6 +504,7 @@ class SQLite3Database extends Database // Named transaction if ($savepoint) { $this->query("ROLLBACK TO $savepoint;"); + $this->transactionDepthDecrease(); return true; } @@ -508,11 +513,11 @@ class SQLite3Database extends Database return false; } - $this->transactionDepthDecrease(); - if ($this->transactionDepth()) { + if ($this->transactionIsNested()) { $this->transactionRollback($this->getTransactionSavepointName()); } else { $this->query('ROLLBACK;'); + $this->transactionDepthDecrease(); } return true; } @@ -528,24 +533,60 @@ class SQLite3Database extends Database if (!$this->transactionDepth()) { return false; } - $this->query('COMMIT;'); - $this->transactionDepthDecrease(); + + if ($this->transactionIsNested()) { + $savepoint = $this->getTransactionSavepointName(); + $this->query('RELEASE ' . $savepoint); + $this->transactionDepthDecrease(); + } else { + $this->query('COMMIT;'); + $this->resetTransactionNesting(); + } + + if ($chain) { + $this->transactionStart(); + } + return true; } /** - * Increase the nested transaction level by one + * Indicate whether or not the current transaction is nested + * Returns false if there are no transactions, or the open + * transaction is the 'outer' transaction, i.e. not nested. + * + * @return bool */ - protected function transactionDepthIncrease() + protected function transactionIsNested() + { + return $this->transactionNesting > 1; + } + + /** + * Increase the nested transaction level by one + * savepoint tracking is optional because BEGIN + * opens a transaction, but is not a named reference + * + * @param string $savepoint + */ + protected function transactionDepthIncrease($savepoint = null) { ++$this->transactionNesting; + if ($savepoint) { + array_push($this->transactionSavepoints, $savepoint); + } } /** * Decrease the nested transaction level by one + * and reduce the savepoint tracking if we are + * nesting, as the last one is no longer valid */ protected function transactionDepthDecrease() { + if ($this->transactionIsNested()) { + array_pop($this->transactionSavepoints); + } --$this->transactionNesting; } @@ -555,6 +596,7 @@ class SQLite3Database extends Database protected function resetTransactionNesting() { $this->transactionNesting = 0; + $this->transactionSavepoints = []; } public function query($sql, $errorLevel = E_USER_ERROR) From 5eacbe7842bd6cd7936d1e329022d29b457a1e83 Mon Sep 17 00:00:00 2001 From: NightjarNZ Date: Tue, 16 Oct 2018 21:57:51 +1300 Subject: [PATCH 4/5] FIX convert index definitions to reflect actual support It is not uncommon for an index to be defined as e.g. 'fulltext' which SQLite3 does not support without a module to create a virtual table (rather than an index on an existing one). The code already in place sees that definitions be updated to 'index' on the fly during creation and later inspection (indexList) - which causes issue when comparing existing table definitions to what SilverStripe expects by DataObject configuration. This discrepency leads to tables constantly being marked to update, although effectively nothing actually changes. We can save these CPU cycles and a bit of head scratching by converting to a supported index type. --- code/SQLite3Database.php | 2 +- code/SQLite3SchemaManager.php | 13 +++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/code/SQLite3Database.php b/code/SQLite3Database.php index 07942ca..b6cbd9a 100644 --- a/code/SQLite3Database.php +++ b/code/SQLite3Database.php @@ -491,7 +491,7 @@ class SQLite3Database extends Database /** * Fetch the name of the most recent savepoint - * + * * @return string */ protected function getTransactionSavepointName() diff --git a/code/SQLite3SchemaManager.php b/code/SQLite3SchemaManager.php index a993dc7..119085b 100644 --- a/code/SQLite3SchemaManager.php +++ b/code/SQLite3SchemaManager.php @@ -286,7 +286,7 @@ class SQLite3SchemaManager extends DBSchemaManager // Then alter the table column $database = $this->database; - $database->withTransaction(function() use ($database, $queries, $indexList) { + $database->withTransaction(function () use ($database, $queries, $indexList) { foreach ($queries as $query) { $database->query($query . ';'); } @@ -330,7 +330,7 @@ class SQLite3SchemaManager extends DBSchemaManager // Then alter the table column $database = $this->database; - $database->withTransaction(function() use ($database, $queries) { + $database->withTransaction(function () use ($database, $queries) { foreach ($queries as $query) { $database->query($query . ';'); } @@ -431,6 +431,15 @@ class SQLite3SchemaManager extends DBSchemaManager return $this->buildSQLiteIndexName($table, $index); } + protected function convertIndexSpec($indexSpec) + { + $supportedIndexTypes = ['index', 'unique']; + if (isset($indexSpec['type']) && !in_array($indexSpec['type'], $supportedIndexTypes)) { + $indexSpec['type'] = 'index'; + } + return parent::convertIndexSpec($indexSpec); + } + public function indexList($table) { $indexList = array(); From c2569099cea317d6f2cefd281a3153e8671670a1 Mon Sep 17 00:00:00 2001 From: NightjarNZ Date: Thu, 18 Oct 2018 22:12:06 +1300 Subject: [PATCH 5/5] correct @deprecated docblock to be Draft PSR-5 compliant --- code/SQLite3Database.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/SQLite3Database.php b/code/SQLite3Database.php index b6cbd9a..0575019 100644 --- a/code/SQLite3Database.php +++ b/code/SQLite3Database.php @@ -611,7 +611,7 @@ class SQLite3Database extends Database /** * Inspect a SQL query prior to execution - * @deprecated 2..3 + * @deprecated 2.2.0:3.0.0 * @param string $sql */ protected function inspectQuery($sql)