rdbms: do not silently rollback empty transactions on error

Since there might be important view snapshots, temp tables, or effects
from SET statements or the like, go into TRX_ERROR state for "possible
transaction level errors" even if no recognized writes took place and
the transaction was not explicit.

Change-Id: I32c34bc28b845e343d0167a220412824838eaed8

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index aeda5b9..152d314 100644 (file)
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -1169,27 +1169,21 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 
                if ( $ret === false ) {
                        if ( $this->trxLevel ) {
-                               if ( !$this->wasKnownStatementRollbackError() ) {
-                                       # Either the query was aborted or all queries after BEGIN where aborted.
-                                       if ( $this->explicitTrxActive() || $priorWritesPending ) {
-                                               # In the first case, the only options going forward are (a) ROLLBACK, or
-                                               # (b) ROLLBACK TO SAVEPOINT (if one was set). If the later case, the only
-                                               # option is ROLLBACK, since the snapshots would have been released.
-                                               $this->trxStatus = self::STATUS_TRX_ERROR;
-                                               $this->trxStatusCause =
-                                                       $this->makeQueryException( $lastError, $lastErrno, $sql, $fname );
-                                               $tempIgnore = false; // cannot recover
-                                       } else {
-                                               # Nothing prior was there to lose from the transaction,
-                                               # so just roll it back.
-                                               $this->rollback( __METHOD__ . " ($fname)", self::FLUSHING_INTERNAL );
-                                       }
-                                       $this->trxStatusIgnoredCause = null;
-                               } else {
+                               if ( $this->wasKnownStatementRollbackError() ) {
                                        # We're ignoring an error that caused just the current query to be aborted.
-                                       # But log the cause so we can log a deprecation notice if a
-                                       # caller actually does ignore it.
+                                       # But log the cause so we can log a deprecation notice if a caller actually
+                                       # does ignore it.
                                        $this->trxStatusIgnoredCause = [ $lastError, $lastErrno, $fname ];
+                               } else {
+                                       # Either the query was aborted or all queries after BEGIN where aborted.
+                                       # In the first case, the only options going forward are (a) ROLLBACK, or
+                                       # (b) ROLLBACK TO SAVEPOINT (if one was set). If the later case, the only
+                                       # option is ROLLBACK, since the snapshots would have been released.
+                                       $this->trxStatus = self::STATUS_TRX_ERROR;
+                                       $this->trxStatusCause =
+                                               $this->makeQueryException( $lastError, $lastErrno, $sql, $fname );
+                                       $tempIgnore = false; // cannot recover
+                                       $this->trxStatusIgnoredCause = null;
                                }
                        }
 

diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
index b434396..84c0c1c 100644 (file)
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
@@ -1893,15 +1893,31 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase {
 
                $this->database->setFlag( Database::DBO_TRX );
 
-               // Implicit transaction gets silently rolled back
+               // Implicit transaction does not get silently rolled back
                $this->database->begin( __METHOD__, Database::TRANSACTION_INTERNAL );
                call_user_func( $doError );
-               $this->database->delete( 'x', [ 'field' => 1 ], __METHOD__ );
-               $this->database->commit( __METHOD__, Database::FLUSHING_INTERNAL );
-               // phpcs:ignore
-               $this->assertLastSql( 'BEGIN; DELETE FROM error WHERE 1; ROLLBACK; BEGIN; DELETE FROM x WHERE field = \'1\'; COMMIT' );
+               try {
+                       $this->database->delete( 'x', [ 'field' => 1 ], __METHOD__ );
+                       $this->fail( 'Expected exception not thrown' );
+               } catch ( DBTransactionError $e ) {
+                       $this->assertEquals(
+                               'Cannot execute query from ' . __METHOD__ . ' while transaction status is ERROR.',
+                               $e->getMessage()
+                       );
+               }
+               try {
+                       $this->database->commit( __METHOD__, Database::FLUSHING_INTERNAL );
+                       $this->fail( 'Expected exception not thrown' );
+               } catch ( DBTransactionError $e ) {
+                       $this->assertEquals(
+                               'Cannot execute query from ' . __METHOD__ . ' while transaction status is ERROR.',
+                               $e->getMessage()
+                       );
+               }
+               $this->database->rollback( __METHOD__, Database::FLUSHING_INTERNAL );
+               $this->assertLastSql( 'BEGIN; DELETE FROM error WHERE 1; ROLLBACK' );
 
-               // ... unless there were prior writes
+               // Likewise if there were prior writes
                $this->database->begin( __METHOD__, Database::TRANSACTION_INTERNAL );
                $this->database->delete( 'x', [ 'field' => 1 ], __METHOD__ );
                call_user_func( $doError );