Aaron Schulz has uploaded a new change for review.
https://gerrit.wikimedia.org/r/304687
Change subject: Make transaction enforcement stricter
......................................................................
Make transaction enforcement stricter
* Nested begin() will fail for DBO_TRX transactions too,
even if no writes happened yet. This avoids snapshot
less from REPEATABLE-READ or SSI SERIALABLE (postgres).
* Warn when begin() happens and DBO_TRX is set. This is
not meant to happen as it can make multi-DB commits
less robust. 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, 23 insertions(+), 42 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/87/304687/1
diff --git a/includes/db/DBConnRef.php b/includes/db/DBConnRef.php
index 53862b9..2ad5e8c 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 = '' ) {
return $this->__call( __FUNCTION__, func_get_args() );
}
diff --git a/includes/db/Database.php b/includes/db/Database.php
index 3ea2cda..e01ecb9 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,21 @@
$this->endAtomic( $fname );
}
- final public function begin( $fname = __METHOD__ ) {
- if ( $this->mTrxLevel ) { // implicit commit
+ final public function begin( $fname = __METHOD__, $mode = '' ) {
+ if ( $this->mTrxLevel ) {
+ // Protect against mismatched atomic section,
transaction nesting, and snapshot loss
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.";
} 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}).";
+ } else {
+ $msg = "$fname: Implicit transaction already
active (from {$this->mTrxFname}).";
}
-
- $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 );
+ throw new DBUnexpectedError( $this, $msg );
+ } elseif ( $this->getFlag( DBO_TRX ) && $mode !==
self::TRANSACTION_INTERNAL ) {
+ // @TODO: make this an exception for multi-DB commit
safety
+ wfLogDBError( "$fname: Implicit transaction expected
(DBO_TRX set)." );
}
// Avoid fatals if close() was called
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..5bbd151 100644
--- a/includes/db/IDatabase.php
+++ b/includes/db/IDatabase.php
@@ -40,6 +40,8 @@
/** @var int Callback triggered by rollback */
const TRIGGER_ROLLBACK = 3;
+ /** @var string Transaction is started 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 +1364,10 @@
* automatically because of the DBO_TRX flag.
*
* @param string $fname
+ * @param string $mode A situationally valid IDatabase::TRANSACTION_*
constant
* @throws DBError
*/
- public function begin( $fname = __METHOD__ );
+ public function begin( $fname = __METHOD__, $mode = '' );
/**
* 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: newchange
Gerrit-Change-Id: I31b8b1b112cf9110b805a023732bce4dcff0604c
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
