From a26fbb67054a09e80ac7e59c1a9f351474649a1e Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sat, 13 Aug 2016 21:26:12 -0700 Subject: [PATCH] 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 --- includes/db/DBConnRef.php | 2 +- includes/db/Database.php | 81 ++++++++++++++------------------ includes/db/DatabasePostgres.php | 4 +- includes/db/IDatabase.php | 8 +++- 4 files changed, 44 insertions(+), 51 deletions(-) diff --git a/includes/db/DBConnRef.php b/includes/db/DBConnRef.php index 53862b96da..4a7836355a 100644 --- 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 78975fff17..d492def475 100644 --- 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 867aeb8705..1ecdd26088 100644 --- 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 af024b8517..fce5aa89e9 100644 --- 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(). -- 2.20.1