Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Make transaction enforcement stricter

Browse source 
* 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
This commit is contained in:
Aaron Schulz 2016-08-13 21:26:12 -07:00
parent 06045fd914
commit a26fbb6705
4 changed files with 44 additions and 51 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
includes/db/DBConnRef.php
View file

@ -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() );
}

81
includes/db/Database.php
View file

@ -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"
);
$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->runOnTransactionPreCommitCallbacks();
$writeTime = $this->pendingWriteQueryDuration();
$this->doCommit( $fname );
if ( $this->mTrxDoneWrites ) {
$this->mDoneWrites = microtime( true );
$this->getTransactionProfiler()->transactionWritingOut(
$this->mServer, $this->mDBname, $this->mTrxShortId, $writeTime );
}
$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)."
);
}
}

4
includes/db/DatabasePostgres.php
View file

@ -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;

8
includes/db/IDatabase.php
View file

@ -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().