jenkins-bot has submitted this change and it was merged.
Change subject: Assorted transaction logic cleanups
......................................................................
Assorted transaction logic cleanups
* Replace DB begin()/commit() with startAtomic()/endAtomic() as needed.
* Avoid using rollback(), which generates warnings, and instead doing a
SELECT FOR UPDATE and checking if the user exists instead of counting
the affected rows of the subsequent UPDATE.
* Block rename jobs until the original transaction commits and abort
jobs if the log row was not found. That row acts as a good proxy for
whether the transaction committed or not. This is better than simply
moving the job insertion to post-commit because that breaks if the job
queue is down but not the DB.
* The User::invalidateCache() already defers to pre-commit with HOLDOFF_TTL
in clearSharedCache(), so having two of the calls was redundant. The later
call was removed.
Bug: T120791
Change-Id: I33f314fb93228b090c1b82c55b1621764e072028
---
M RenameUserJob.php
M RenameuserSQL.php
2 files changed, 84 insertions(+), 33 deletions(-)
Approvals:
Legoktm: Looks good to me, approved
jenkins-bot: Verified
diff --git a/RenameUserJob.php b/RenameUserJob.php
index e483733..bfe6d4a 100644
--- a/RenameUserJob.php
+++ b/RenameUserJob.php
@@ -9,6 +9,7 @@
* - oldname : The old user name
* - newname : The new user name
* - count : The expected number of rows to update in this batch
+ * - logId : The ID of the logging table row expected to exist if the
rename was committed
*
* Additionally, one of the following groups of parameters must be set:
* a) The timestamp based rename paramaters:
@@ -55,8 +56,29 @@
}
$uniqueKey = isset( $this->params['uniqueKey'] ) ?
$this->params['uniqueKey'] : null;
$keyId = isset( $this->params['keyId'] ) ?
$this->params['keyId'] : null;
+ $logId = isset( $this->params['logId'] ) ?
$this->params['logId'] : null;
$dbw = wfGetDB( DB_MASTER );
+ if ( $logId ) {
+ # Block until the transaction that inserted this job
commits.
+ # The atomic section is for sanity as FOR UPDATE does
not lock in auto-commit mode
+ # per
http://dev.mysql.com/doc/refman/5.7/en/innodb-locking-reads.html.
+ $dbw->startAtomic( __METHOD__ );
+ $committed = $dbw->selectField( 'logging',
+ '1',
+ array( 'log_id' => $logId ),
+ __METHOD__,
+ array( 'FOR UPDATE' )
+ );
+ $dbw->endAtomic( __METHOD__ );
+ # If the transaction inserting this job was rolled
back, detect that
+ if ( $committed === false ) { // rollback happened?
+ throw new LogicException( "Cannot run job if
the account rename failed." );
+ }
+ }
+
+ # Flush any state snapshot data (and release the lock above)
+ $dbw->commit( __METHOD__, 'flush' );
# Conditions like "*_user_text = 'x'
$conds = array( $column => $oldname );
diff --git a/RenameuserSQL.php b/RenameuserSQL.php
index 9fbacda..57dad4e 100755
--- a/RenameuserSQL.php
+++ b/RenameuserSQL.php
@@ -165,12 +165,19 @@
$contribs = User::newFromId( $this->uid )->getEditCount();
$dbw = wfGetDB( DB_MASTER );
- $dbw->begin();
+ $dbw->startAtomic( __METHOD__ );
+
Hooks::run( 'RenameUserPreRename', array( $this->uid,
$this->old, $this->new ) );
- // Rename and touch the user before re-attributing edits,
- // this avoids users still being logged in and making new edits
while
- // being renamed, which leaves edits at the old name.
+ // Make sure the user exists if needed
+ if ( $this->checkIfUserExists && !self::lockUserAndGetId(
$this->old ) ) {
+ $this->debug( "User {$this->old} does not exist,
bailing out" );
+
+ return false;
+ }
+
+ // Rename and touch the user before re-attributing edits to
avoid users still being
+ // logged in and making new edits (under the old name) while
being renamed.
$this->debug( "Starting rename of {$this->old} to {$this->new}"
);
$dbw->update( 'user',
array( 'user_name' => $this->new, 'user_touched' =>
$dbw->timestamp() ),
@@ -178,27 +185,21 @@
__METHOD__
);
- if ( !$dbw->affectedRows() && $this->checkIfUserExists ) {
- $dbw->rollback();
- $this->debug( "User {$this->old} does not exist,
bailing out" );
-
- return false;
- }
-
// Reset token to break login with central auth systems.
// Again, avoids user being logged in with old name.
$user = User::newFromId( $this->uid );
$authUser = $wgAuth->getUserInstance( $user );
$authUser->resetAuthToken();
- // Delete from memcached.
+ // Purge user cache
$user->invalidateCache();
// Update ipblock list if this user has a block in there.
$dbw->update( 'ipblocks',
array( 'ipb_address' => $this->new ),
array( 'ipb_user' => $this->uid, 'ipb_address' =>
$this->old ),
- __METHOD__ );
+ __METHOD__
+ );
// Update this users block/rights log. Ideally, the logs would
be historical,
// but it is really annoying when users have "clean" block logs
by virtue of
// being renamed, which makes admin tasks more of a pain...
@@ -216,8 +217,10 @@
array( 'log_type' => $logTypesOnUser,
'log_namespace' => NS_USER,
'log_title' => $oldTitle->getDBkey() ),
- __METHOD__ );
- // Do immediate updates!
+ __METHOD__
+ );
+
+ // Do immediate re-attribution table updates...
foreach ( $this->tables as $table => $fieldSet ) {
list( $nameCol, $userCol ) = $fieldSet;
$dbw->update( $table,
@@ -227,6 +230,7 @@
);
}
+ /** @var RenameUserJob[] $jobs */
$jobs = array(); // jobs for all tables
// Construct jobqueue updates...
// FIXME: if a bureaucrat renames a user in error, he/she
@@ -294,23 +298,6 @@
$dbw->freeResult( $res );
}
- $count = count( $jobs );
- if ( $count > 0 ) {
- JobQueueGroup::singleton()->push( $jobs,
JobQueue::QOS_ATOMIC );
- $this->debug( "Queued $count jobs for {$this->old} to
{$this->new}" );
- }
-
- // Commit the transaction
- $dbw->commit();
-
- // Delete from memcached again to make sure
- $user->invalidateCache();
-
- // Clear caches and inform authentication plugins
- $user = User::newFromId( $this->uid );
- $wgAuth->updateExternalDB( $user );
- Hooks::run( 'RenameUserComplete', array( $this->uid,
$this->old, $this->new ) );
-
// Log it!
$logEntry = new ManualLogEntry( 'renameuser', 'renameuser' );
$logEntry->setPerformer( $this->renamer );
@@ -322,10 +309,52 @@
'6::edits' => $contribs
) );
$logid = $logEntry->insert();
- $logEntry->publish( $logid );
+ // Include the log_id in the jobs as a DB commit marker
+ foreach ( $jobs as $job ) {
+ $job->params['logId'] = $logid;
+ }
+
+ // Insert any jobs as needed. If this fails, then an exception
will be thrown and the
+ // DB transaction will be rolled back. If it succeeds but the
DB commit fails, then the
+ // jobs will see that the transaction was not committed and
will cancel themselves.
+ $count = count( $jobs );
+ if ( $count > 0 ) {
+ JobQueueGroup::singleton()->push( $jobs,
JobQueue::QOS_ATOMIC );
+ $this->debug( "Queued $count jobs for {$this->old} to
{$this->new}" );
+ }
+
+ // Commit the transaction
+ $dbw->endAtomic( __METHOD__ );
+
+ $that = $this;
+ $dbw->onTransactionIdle( function() use ( $that, $dbw,
$logEntry, $logid ) {
+ global $wgAuth;
+ // Keep any updates here in a transaction
+ $dbw->setFlag( DBO_TRX );
+ // Clear caches and inform authentication plugins
+ $user = User::newFromId( $that->uid );
+ $wgAuth->updateExternalDB( $user );
+ Hooks::run( 'RenameUserComplete', array( $this->uid,
$this->old, $this->new ) );
+ // Publish to RC
+ $logEntry->publish( $logid );
+ } );
$this->debug( "Finished rename for {$this->old} to
{$this->new}" );
return true;
}
+
+ /**
+ * @param string $name Current wiki local user name
+ * @return integer Returns 0 if no row was found
+ */
+ private static function lockUserAndGetId( $name ) {
+ return (int)wfGetDB( DB_MASTER )->selectField(
+ 'user',
+ 'user_id',
+ array( 'user_name' => $name ),
+ __METHOD__,
+ array( 'FOR UPDATE' )
+ );
+ }
}
--
To view, visit https://gerrit.wikimedia.org/r/257746
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I33f314fb93228b090c1b82c55b1621764e072028
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Renameuser
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Legoktm <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
