From 0b5ed025e97700d4d9c23e2978d9c9e14bc8b7fe Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Sun, 22 Apr 2018 15:38:49 -0700
Subject: [PATCH] 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
---
 includes/libs/rdbms/database/Database.php     | 32 ++++++++-----------
 .../libs/rdbms/database/DatabaseSQLTest.php   | 28 ++++++++++++----
 2 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index aeda5b9ce0..152d314f65 100644
--- 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 b434396173..84c0c1c4db 100644
--- 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 );
-- 
2.20.1