jenkins-bot has submitted this change and it was merged.
Change subject: Make transaction enforcement stricter
......................................................................
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
---
M includes/db/DBConnRef.php
M includes/db/Database.php
M includes/db/DatabasePostgres.php
M includes/db/IDatabase.php
4 files changed, 44 insertions(+), 51 deletions(-)
Approvals:
Krinkle: Looks good to me, approved
jenkins-bot: Verified
diff --git a/includes/db/DBConnRef.php b/includes/db/DBConnRef.php
index 53862b9..4a78363 100644
--- a/includes/db/DBConnRef.php
+++ b/includes/db/DBConnRef.php
@@ -445,7 +445,7 @@
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
--- a/includes/db/Database.php
+++ b/includes/db/Database.php
@@ -815,7 +815,7 @@
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 @@
$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 @@
throw $e;
}
if ( $useTrx ) {
- $this->commit( $fname );
+ $this->commit( $fname, self::TRANSACTION_INTERNAL );
}
return $ok;
@@ -2520,7 +2520,7 @@
$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 @@
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 @@
$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 @@
$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 @@
} 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 @@
}
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..1ecdd2608 100644
--- a/includes/db/DatabasePostgres.php
+++ b/includes/db/DatabasePostgres.php
@@ -149,7 +149,7 @@
$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 @@
* @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
--- a/includes/db/IDatabase.php
+++ b/includes/db/IDatabase.php
@@ -40,6 +40,11 @@
/** @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 @@
* 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().
--
To view, visit https://gerrit.wikimedia.org/r/304687
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I31b8b1b112cf9110b805a023732bce4dcff0604c
Gerrit-PatchSet: 8
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: Parent5446 <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
