Aaron Schulz has uploaded a new change for review.
https://gerrit.wikimedia.org/r/304688
Change subject: Fix premature transactions commit problems
......................................................................
Fix premature transactions commit problems
* Use startAtomic/endAtomic() instead of begin()/commit()
* Now that jobs are pushed pre-commit, add a locking SELECT
to them so they block until the transaction that spawned
them commits. This has the side-effect of making these
operations more atomic since job insertion failure causes
full rollback and rollback after job insertion causes jobs
to abort.
* Avoid using rollback() and use either a dumb insert (mysql)
with a try/catch or a locking SELECT to precheck for key
conflicts.
* Renamed confusing setStatus() to updateStatus().
Bug: T142231
Change-Id: I8677fb5484869edb4bdd9aec77541669feacada9
---
M includes/GlobalRename/GlobalRenameUser.php
M includes/GlobalRename/GlobalRenameUserDatabaseUpdates.php
M includes/GlobalRename/GlobalRenameUserStatus.php
M includes/GlobalRename/GlobalUserMerge.php
M includes/GlobalRename/GlobalUserMergeDatabaseUpdates.php
M includes/LocalRenameJob/LocalRenameJob.php
6 files changed, 57 insertions(+), 24 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CentralAuth
refs/changes/88/304688/1
diff --git a/includes/GlobalRename/GlobalRenameUser.php
b/includes/GlobalRename/GlobalRenameUser.php
index bc2cb06..7bc33f4 100644
--- a/includes/GlobalRename/GlobalRenameUser.php
+++ b/includes/GlobalRename/GlobalRenameUser.php
@@ -138,6 +138,10 @@
$this->oldCAUser->quickInvalidateCache();
$this->newCAUser->quickInvalidateCache();
+ // If job insertion fails, an exception will cause rollback of
all DBs.
+ // The job will block on reading renameuser_status until this
commits due to it using
+ // a locking read and the pending update from
setRenameStatuses() above. If we end up
+ // rolling back, then the job will abort because the status
will not be 'queued'.
$this->injectLocalRenameUserJobs( $wikisAttached, $options );
$this->logger->log(
diff --git a/includes/GlobalRename/GlobalRenameUserDatabaseUpdates.php
b/includes/GlobalRename/GlobalRenameUserDatabaseUpdates.php
index a68186a..7a21912 100644
--- a/includes/GlobalRename/GlobalRenameUserDatabaseUpdates.php
+++ b/includes/GlobalRename/GlobalRenameUserDatabaseUpdates.php
@@ -22,7 +22,7 @@
public function update( $oldname, $newname ) {
$dbw = $this->getDB();
- $dbw->begin( __METHOD__ );
+ $dbw->startAtomic( __METHOD__ );
$dbw->update(
'globaluser',
array( 'gu_name' => $newname ),
@@ -36,6 +36,6 @@
__METHOD__
);
- $dbw->commit( __METHOD__ );
+ $dbw->endAtomic( __METHOD__ );
}
}
diff --git a/includes/GlobalRename/GlobalRenameUserStatus.php
b/includes/GlobalRename/GlobalRenameUserStatus.php
index ecb6101..fc8ec42 100644
--- a/includes/GlobalRename/GlobalRenameUserStatus.php
+++ b/includes/GlobalRename/GlobalRenameUserStatus.php
@@ -139,7 +139,7 @@
* @param string $wiki
* @param string $status
*/
- public function setStatus( $wiki, $status ) {
+ public function updateStatus( $wiki, $status ) {
$dbw = $this->getDB( DB_MASTER );
$nameWhere = $this->getNameWhereClause( $dbw ); // Can be
inlined easily once we require more than 5.3
$fname = __METHOD__;
@@ -164,21 +164,40 @@
public function setStatuses( array $rows ) {
$dbw = $this->getDB( DB_MASTER );
- $dbw->begin( __METHOD__ );
- $dbw->insert(
- 'renameuser_status',
- $rows,
- __METHOD__,
- array( 'IGNORE' )
- );
- if ( $dbw->affectedRows() !== count( $rows ) ) {
- // Race condition, the rename was already started
- $dbw->rollback( __METHOD__ );
- return false;
+ $dbw->startAtomic( __METHOD__ );
+ if ( $dbw->getType() === 'mysql' ) {
+ // If there is duplicate key error, the RDBMs will
rollback the INSERT statement.
+ //
http://dev.mysql.com/doc/refman/5.7/en/innodb-error-handling.html
+ try {
+ $dbw->insert( 'renameuser_status', $rows,
__METHOD__ );
+ $ok = true;
+ } catch ( DBQueryError $e ) {
+ $ok = false;
+ }
+ } else {
+ // At least Postgres does not like continuing after
errors. Only options are
+ // ROLLBACK or COMMIT as is. We could use SAVEPOINT
here, but it's not worth it.
+ $keyConds = [];
+ foreach ( $rows as $row ) {
+ $key = [ 'ru_wiki' => $row->ru_wiki,
'ru_oldname' => $row->ru_oldname ];
+ $keyConds[] = $dbw->makeList( $key, LIST_AND );
+ }
+ // (a) Do a locking check for conflicting rows on the
unique key
+ $ok = !$dbw->selectField(
+ 'renameuser_status',
+ '1',
+ $dbw->makeList( $keyConds, LIST_OR ),
+ __METHOD__,
+ [ 'FOR UPDATE' ]
+ );
+ // (b) Insert the new rows if no conflicts were found
+ if ( $ok ) {
+ $dbw->insert( 'renameuser_status', $rows,
__METHOD__ );
+ }
}
- $dbw->commit( __METHOD__ );
+ $dbw->endAtomic( __METHOD__ );
- return true;
+ return $ok;
}
/**
diff --git a/includes/GlobalRename/GlobalUserMerge.php
b/includes/GlobalRename/GlobalUserMerge.php
index 60066fd..49a99c4 100644
--- a/includes/GlobalRename/GlobalUserMerge.php
+++ b/includes/GlobalRename/GlobalUserMerge.php
@@ -98,7 +98,7 @@
public function merge( $reason ) {
$wikis = array();
- foreach( $this->oldCAUsers as $oldCAUser ) {
+ foreach ( $this->oldCAUsers as $oldCAUser ) {
$oldWikis = $oldCAUser->listAttached();
foreach ( $oldWikis as $wiki ) {
$wikis[$wiki][] = $oldCAUser->getName();
@@ -136,7 +136,10 @@
}
$this->clearCaches();
-
+ // If job insertion fails, an exception will cause rollback of
all DBs.
+ // The job will block on reading renameuser_status until this
commits due to it using
+ // a locking read and the pending update from
setRenameStatuses() above. If we end up
+ // rolling back, then the job will abort because the status
will not be 'queued'.
$this->injectJobs( $wikis );
$this->addLogEntry( $reason );
diff --git a/includes/GlobalRename/GlobalUserMergeDatabaseUpdates.php
b/includes/GlobalRename/GlobalUserMergeDatabaseUpdates.php
index dc477cf..c592233 100644
--- a/includes/GlobalRename/GlobalUserMergeDatabaseUpdates.php
+++ b/includes/GlobalRename/GlobalUserMergeDatabaseUpdates.php
@@ -25,7 +25,7 @@
public function merge( $oldname, $newname ) {
$dbw = $this->getDB();
- $dbw->begin( __METHOD__ );
+ $dbw->startAtomic( __METHOD__ );
// Delete the old user's globaluser row
$dbw->delete(
'globaluser',
@@ -53,7 +53,7 @@
array( 'lu_name' => $oldname )
);
- $dbw->commit( __METHOD__ );
+ $dbw->endAtomic( __METHOD__ );
}
/**
@@ -82,6 +82,7 @@
*/
public function mergeGlobalUserGroups( $fromId, $toId ) {
$dbw = $this->getDB();
+ $dbw->startAtomic( __METHOD__ );
$dbw->update(
'global_user_groups',
array( 'gug_user' => $toId ),
@@ -95,5 +96,6 @@
array( 'gug_user' => $fromId ),
__METHOD__
);
+ $dbw->endAtomic( __METHOD__ );
}
}
diff --git a/includes/LocalRenameJob/LocalRenameJob.php
b/includes/LocalRenameJob/LocalRenameJob.php
index 0eb0c88..e5d7266 100644
--- a/includes/LocalRenameJob/LocalRenameJob.php
+++ b/includes/LocalRenameJob/LocalRenameJob.php
@@ -17,8 +17,10 @@
public function run() {
$this->setRenameUserStatus( new GlobalRenameUserStatus(
$this->params['to'] ) );
- // bail if it's already done or in progress
- $status = $this->renameuserStatus->getStatus(
GlobalRenameUserStatus::READ_LATEST );
+ // Bail if it's already done or in progress. Use a locking read
to block until the
+ // transaction adding this job is done, so we can see its
changes. This is similar to
+ // the trick the the RenameUser extension does.
+ $status = $this->renameuserStatus->getStatus(
GlobalRenameUserStatus::READ_LOCKING );
if ( $status !== 'queued' && $status !== 'failed' ) {
LoggerFactory::getInstance( 'rename' )->info( 'skipping
duplicate rename from {user}', [
'user' => $this->params['from'],
@@ -27,6 +29,10 @@
] );
return true;
}
+ // Clear any REPEATABLE-READ snapshot in case the READ_LOCKING
blocked above. We want
+ // regular non-locking SELECTs to see all the changes from that
transaction we waited on.
+ $factory =
MediaWikiServices::getInstance()->getDBLoadBalancerFactory();
+ $factory->commitMasterChanges( __METHOD__ );
if ( isset( $this->params['session'] ) ) {
// Don't carry over users or sessions because it's
going to be wrong
@@ -43,7 +49,6 @@
$this->addTeardownCallback( [ $this, 'scheduleNextWiki'
] );
} catch ( Exception $e ) {
// This will lock the user out of their account until a
sysadmin intervenes
- $factory =
MediaWikiServices::getInstance()->getDBLoadBalancerFactory();
$factory->rollbackMasterChanges( __METHOD__ );
$this->updateStatus( 'failed' );
$factory->commitMasterChanges( __METHOD__ );
@@ -103,7 +108,7 @@
}
protected function updateStatus( $status ) {
- $this->renameuserStatus->setStatus( wfWikiID(), $status );
+ $this->renameuserStatus->updateStatus( wfWikiID(), $status );
}
protected function scheduleNextWiki() {
--
To view, visit https://gerrit.wikimedia.org/r/304688
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8677fb5484869edb4bdd9aec77541669feacada9
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/CentralAuth
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
