Make transaction enforcement stricter

* Nested begin()/commit() will no-op when inside active
  DBO_TRX transactions, even if no writes happened yet.
  This avoids snapshot loss from REPEATABLE-READ or SSI
  SERIALABLE (postgres). An error will be logged.
* Also no-op when begin() happens, no transaction started,
  but DBO_TRX is set. That should not happen as it can make
  multi-DB commits less robust and can clear snapshots early.
  An error will be logged.
* Throw an error when explicit rollback() is used and DBO_TRX
  is set. This will cause peer DBs to rollback too via the
  logic in MWExceptionHander.
* Callers should use LBFactory methods for doing commit/rollback
  instead.

Change-Id: I31b8b1b112cf9110b805a023732bce4dcff0604c

diff --git a/includes/db/DBConnRef.php b/includes/db/DBConnRef.php
index 53862b9..4a78363 100644 (file)
--- a/includes/db/DBConnRef.php
+++ b/includes/db/DBConnRef.php
@@ -445,7 +445,7 @@ class DBConnRef implements IDatabase {
                return $this->__call( __FUNCTION__, func_get_args() );
        }
 
-       public function begin( $fname = __METHOD__ ) {
+       public function begin( $fname = __METHOD__, $mode = IDatabase::TRANSACTION_EXPLICIT ) {
                return $this->__call( __FUNCTION__, func_get_args() );
        }
 

diff --git a/includes/db/Database.php b/includes/db/Database.php
index 78975ff..d492def 100644 (file)
--- a/includes/db/Database.php
+++ b/includes/db/Database.php
@@ -815,7 +815,7 @@ abstract class DatabaseBase implements IDatabase {
                if ( !$this->mTrxLevel && $this->getFlag( DBO_TRX )
                        && $this->isTransactableQuery( $sql )
                ) {
-                       $this->begin( __METHOD__ . " ($fname)" );
+                       $this->begin( __METHOD__ . " ($fname)", self::TRANSACTION_INTERNAL );
                        $this->mTrxAutomatic = true;
                }
 
@@ -2203,7 +2203,7 @@ abstract class DatabaseBase implements IDatabase {
 
                $useTrx = !$this->mTrxLevel;
                if ( $useTrx ) {
-                       $this->begin( $fname );
+                       $this->begin( $fname, self::TRANSACTION_INTERNAL );
                }
                try {
                        # Update any existing conflicting row(s)
@@ -2221,7 +2221,7 @@ abstract class DatabaseBase implements IDatabase {
                        throw $e;
                }
                if ( $useTrx ) {
-                       $this->commit( $fname );
+                       $this->commit( $fname, self::TRANSACTION_INTERNAL );
                }
 
                return $ok;
@@ -2520,7 +2520,7 @@ abstract class DatabaseBase implements IDatabase {
                        $this->mTrxPreCommitCallbacks[] = [ $callback, wfGetCaller() ];
                } else {
                        // If no transaction is active, then make one for this callback
-                       $this->begin( __METHOD__ );
+                       $this->begin( __METHOD__, self::TRANSACTION_INTERNAL );
                        try {
                                call_user_func( $callback );
                                $this->commit( __METHOD__ );
@@ -2628,7 +2628,7 @@ abstract class DatabaseBase implements IDatabase {
 
        final public function startAtomic( $fname = __METHOD__ ) {
                if ( !$this->mTrxLevel ) {
-                       $this->begin( $fname );
+                       $this->begin( $fname, self::TRANSACTION_INTERNAL );
                        $this->mTrxAutomatic = true;
                        // If DBO_TRX is set, a series of startAtomic/endAtomic pairs will result
                        // in all changes being in one transaction to keep requests transactional.
@@ -2666,43 +2666,26 @@ abstract class DatabaseBase implements IDatabase {
                $this->endAtomic( $fname );
        }
 
-       final public function begin( $fname = __METHOD__ ) {
-               if ( $this->mTrxLevel ) { // implicit commit
+       final public function begin( $fname = __METHOD__, $mode = self::TRANSACTION_EXPLICIT ) {
+               // Protect against mismatched atomic section, transaction nesting, and snapshot loss
+               if ( $this->mTrxLevel ) {
                        if ( $this->mTrxAtomicLevels ) {
-                               // If the current transaction was an automatic atomic one, then we definitely have
-                               // a problem. Same if there is any unclosed atomic level.
                                $levels = implode( ', ', $this->mTrxAtomicLevels );
-                               throw new DBUnexpectedError(
-                                       $this,
-                                       "Got explicit BEGIN from $fname while atomic section(s) $levels are open."
-                               );
+                               $msg = "Got explicit BEGIN from $fname while atomic section(s) $levels are open.";
+                               throw new DBUnexpectedError( $this, $msg );
                        } elseif ( !$this->mTrxAutomatic ) {
-                               // We want to warn about inadvertently nested begin/commit pairs, but not about
-                               // auto-committing implicit transactions that were started by query() via DBO_TRX
-                               throw new DBUnexpectedError(
-                                       $this,
-                                       "$fname: Transaction already in progress (from {$this->mTrxFname}), " .
-                                               " performing implicit commit!"
-                               );
-                       } elseif ( $this->mTrxDoneWrites ) {
-                               // The transaction was automatic and has done write operations
-                               throw new DBUnexpectedError(
-                                       $this,
-                                       "$fname: Automatic transaction with writes in progress" .
-                                               " (from {$this->mTrxFname}), performing implicit commit!\n"
-                               );
-                       }
-
-                       $this->runOnTransactionPreCommitCallbacks();
-                       $writeTime = $this->pendingWriteQueryDuration();
-                       $this->doCommit( $fname );
-                       if ( $this->mTrxDoneWrites ) {
-                               $this->mDoneWrites = microtime( true );
-                               $this->getTransactionProfiler()->transactionWritingOut(
-                                       $this->mServer, $this->mDBname, $this->mTrxShortId, $writeTime );
+                               $msg = "$fname: Explicit transaction already active (from {$this->mTrxFname}).";
+                               throw new DBUnexpectedError( $this, $msg );
+                       } else {
+                               // @TODO: make this an exception at some point
+                               $msg = "$fname: Implicit transaction already active (from {$this->mTrxFname}).";
+                               wfLogDBError( $msg );
+                               return; // join the main transaction set
                        }
-
-                       $this->runOnTransactionIdleCallbacks( self::TRIGGER_COMMIT );
+               } elseif ( $this->getFlag( DBO_TRX ) && $mode !== self::TRANSACTION_INTERNAL ) {
+                       // @TODO: make this an exception at some point
+                       wfLogDBError( "$fname: Implicit transaction expected (DBO_TRX set)." );
+                       return; // let any writes be in the main transaction
                }
 
                // Avoid fatals if close() was called
@@ -2742,7 +2725,7 @@ abstract class DatabaseBase implements IDatabase {
                        $levels = implode( ', ', $this->mTrxAtomicLevels );
                        throw new DBUnexpectedError(
                                $this,
-                               "Got COMMIT while atomic sections $levels are still open"
+                               "Got COMMIT while atomic sections $levels are still open."
                        );
                }
 
@@ -2752,18 +2735,17 @@ abstract class DatabaseBase implements IDatabase {
                        } elseif ( !$this->mTrxAutomatic ) {
                                throw new DBUnexpectedError(
                                        $this,
-                                       "$fname: Flushing an explicit transaction, getting out of sync!"
+                                       "$fname: Flushing an explicit transaction, getting out of sync."
                                );
                        }
                } else {
                        if ( !$this->mTrxLevel ) {
-                               wfWarn( "$fname: No transaction to commit, something got out of sync!" );
+                               wfWarn( "$fname: No transaction to commit, something got out of sync." );
                                return; // nothing to do
                        } elseif ( $this->mTrxAutomatic ) {
-                               throw new DBUnexpectedError(
-                                       $this,
-                                       "$fname: Explicit commit of implicit transaction."
-                               );
+                               // @TODO: make this an exception at some point
+                               wfLogDBError( "$fname: Explicit commit of implicit transaction." );
+                               return; // wait for the main transaction set commit round
                        }
                }
 
@@ -2796,14 +2778,19 @@ abstract class DatabaseBase implements IDatabase {
        }
 
        final public function rollback( $fname = __METHOD__, $flush = '' ) {
-               if ( $flush !== self::FLUSHING_INTERNAL && $flush !== self::FLUSHING_ALL_PEERS ) {
+               if ( $flush === self::FLUSHING_INTERNAL || $flush === self::FLUSHING_ALL_PEERS ) {
                        if ( !$this->mTrxLevel ) {
-                               wfWarn( "$fname: No transaction to rollback, something got out of sync!" );
                                return; // nothing to do
                        }
                } else {
                        if ( !$this->mTrxLevel ) {
+                               wfWarn( "$fname: No transaction to rollback, something got out of sync." );
                                return; // nothing to do
+                       } elseif ( $this->getFlag( DBO_TRX ) ) {
+                               throw new DBUnexpectedError(
+                                       $this,
+                                       "$fname: Expected mass rollback of all peer databases (DBO_TRX set)."
+                               );
                        }
                }
 

diff --git a/includes/db/DatabasePostgres.php b/includes/db/DatabasePostgres.php
index 867aeb8..1ecdd26 100644 (file)
--- a/includes/db/DatabasePostgres.php
+++ b/includes/db/DatabasePostgres.php
@@ -149,7 +149,7 @@ class SavepointPostgres {
                $this->didbegin = false;
                /* If we are not in a transaction, we need to be for savepoint trickery */
                if ( !$dbw->trxLevel() ) {
-                       $dbw->begin( "FOR SAVEPOINT" );
+                       $dbw->begin( "FOR SAVEPOINT", DatabasePostgres::TRANSACTION_INTERNAL );
                        $this->didbegin = true;
                }
        }
@@ -1207,7 +1207,7 @@ __INDEXATTR__;
         * @param string $desiredSchema
         */
        function determineCoreSchema( $desiredSchema ) {
-               $this->begin( __METHOD__ );
+               $this->begin( __METHOD__, self::TRANSACTION_INTERNAL );
                if ( $this->schemaExists( $desiredSchema ) ) {
                        if ( in_array( $desiredSchema, $this->getSchemas() ) ) {
                                $this->mCoreSchema = $desiredSchema;

diff --git a/includes/db/IDatabase.php b/includes/db/IDatabase.php
index af024b8..fce5aa8 100644 (file)
--- a/includes/db/IDatabase.php
+++ b/includes/db/IDatabase.php
@@ -40,6 +40,11 @@ interface IDatabase {
        /** @var int Callback triggered by rollback */
        const TRIGGER_ROLLBACK = 3;
 
+       /** @var string Transaction is requested by regular caller outside of the DB layer */
+       const TRANSACTION_EXPLICIT = '';
+       /** @var string Transaction is requested interally via DBO_TRX/startAtomic() */
+       const TRANSACTION_INTERNAL = 'implicit';
+
        /** @var string Transaction operation comes from service managing all DBs */
        const FLUSHING_ALL_PEERS = 'flush';
        /** @var string Transaction operation comes from the database class internally */
@@ -1362,9 +1367,10 @@ interface IDatabase {
         * automatically because of the DBO_TRX flag.
         *
         * @param string $fname
+        * @param string $mode A situationally valid IDatabase::TRANSACTION_* constant [optional]
         * @throws DBError
         */
-       public function begin( $fname = __METHOD__ );
+       public function begin( $fname = __METHOD__, $mode = self::TRANSACTION_EXPLICIT );
 
        /**
         * Commits a transaction previously started using begin().