From edba39ab602bbcff4aa0f30ce3f7e0dc8bcb6fbb Mon Sep 17 00:00:00 2001 From: Eddie Greiner-Petter Date: Mon, 16 Oct 2017 00:36:40 +0200 Subject: [PATCH] Be more db-friendly when purging expired userrights Each expired row has to be fetched from the user_groups table, deleted from that table and added to the user_former_groups table. Per Jaimes request, let's not do this for all rows at once but for smaller chunks and wait for replication to catch up after each chunk has been processed. In addition the function to purge the expired rows now sets a lock so that there won't be multiple concurrent runs. Also, cleaning this table up isn't urgent and thus should be done in a job and not a deferred update, so let's move it there. Bug: T176754 Change-Id: I671d4b9d09677a2f474477ba7fea33a44d6318aa --- autoload.php | 1 + includes/DefaultSettings.php | 1 + includes/jobqueue/jobs/UserGroupExpiryJob.php | 39 +++++++++++ includes/user/UserGroupMembership.php | 68 ++++++++++++------- tests/phpunit/includes/SiteStatsTest.php | 3 +- 5 files changed, 88 insertions(+), 24 deletions(-) create mode 100644 includes/jobqueue/jobs/UserGroupExpiryJob.php diff --git a/autoload.php b/autoload.php index fc77fcb91c..9042f7b808 100644 --- a/autoload.php +++ b/autoload.php @@ -1591,6 +1591,7 @@ $wgAutoloadLocalClasses = [ 'UserBlockedError' => __DIR__ . '/includes/exception/UserBlockedError.php', 'UserCache' => __DIR__ . '/includes/cache/UserCache.php', 'UserDupes' => __DIR__ . '/maintenance/userDupes.inc', + 'UserGroupExpiryJob' => __DIR__ . '/includes/jobqueue/jobs/UserGroupExpiryJob.php', 'UserGroupMembership' => __DIR__ . '/includes/user/UserGroupMembership.php', 'UserMailer' => __DIR__ . '/includes/mail/UserMailer.php', 'UserNamePrefixSearch' => __DIR__ . '/includes/user/UserNamePrefixSearch.php', diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index a6a36865bb..772e07671a 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -7459,6 +7459,7 @@ $wgJobClasses = [ 'clearUserWatchlist' => ClearUserWatchlistJob::class, 'cdnPurge' => CdnPurgeJob::class, 'enqueue' => EnqueueJob::class, // local queue for multi-DC setups + 'userGroupExpiry' => UserGroupExpiryJob::class, 'null' => NullJob::class, ]; diff --git a/includes/jobqueue/jobs/UserGroupExpiryJob.php b/includes/jobqueue/jobs/UserGroupExpiryJob.php new file mode 100644 index 0000000000..0945e58f16 --- /dev/null +++ b/includes/jobqueue/jobs/UserGroupExpiryJob.php @@ -0,0 +1,39 @@ +removeDuplicates = true; + } + + /** + * Run the job + * @return bool Success + */ + public function run() { + UserGroupMembership::purgeExpired(); + + return true; + } +} diff --git a/includes/user/UserGroupMembership.php b/includes/user/UserGroupMembership.php index f771f4285e..e757e59629 100644 --- a/includes/user/UserGroupMembership.php +++ b/includes/user/UserGroupMembership.php @@ -21,6 +21,7 @@ */ use Wikimedia\Rdbms\IDatabase; +use MediaWiki\MediaWikiServices; /** * Represents a "user group membership" -- a specific instance of a user belonging @@ -158,7 +159,7 @@ class UserGroupMembership { } // Purge old, expired memberships from the DB - self::purgeExpired( $dbw ); + JobQueueGroup::singleton()->push( new UserGroupExpiryJob() ); // Check that the values make sense if ( $this->group === null ) { @@ -236,38 +237,59 @@ class UserGroupMembership { /** * Purge expired memberships from the user_groups table - * - * @param IDatabase|null $dbw */ - public static function purgeExpired( IDatabase $dbw = null ) { - if ( wfReadOnly() ) { + public static function purgeExpired() { + $services = MediaWikiServices::getInstance(); + if ( $services->getReadOnlyMode()->isReadOnly() ) { return; } - if ( $dbw === null ) { - $dbw = wfGetDB( DB_MASTER ); - } + $lbFactory = $services->getDBLoadBalancerFactory(); + $ticket = $lbFactory->getEmptyTransactionTicket( __METHOD__ ); + $dbw = $services->getDBLoadBalancer()->getConnection( DB_MASTER ); - DeferredUpdates::addUpdate( new AtomicSectionUpdate( - $dbw, - __METHOD__, - function ( IDatabase $dbw, $fname ) { - $expiryCond = [ 'ug_expiry < ' . $dbw->addQuotes( $dbw->timestamp() ) ]; - $res = $dbw->select( 'user_groups', self::selectFields(), $expiryCond, $fname ); + $lockKey = $dbw->getDomainID() . ':usergroups-prune'; // specific to this wiki + $scopedLock = $dbw->getScopedLockAndFlush( $lockKey, __METHOD__, 0 ); + if ( !$scopedLock ) { + return; // already running + } - // save an array of users/groups to insert to user_former_groups - $usersAndGroups = []; + $now = time(); + do { + $dbw->startAtomic( __METHOD__ ); + + $res = $dbw->select( + 'user_groups', + self::selectFields(), + [ 'ug_expiry < ' . $dbw->addQuotes( $dbw->timestamp( $now ) ) ], + __METHOD__, + [ 'FOR UPDATE', 'LIMIT' => 100 ] + ); + + if ( $res->numRows() > 0 ) { + $insertData = []; // array of users/groups to insert to user_former_groups + $deleteCond = []; // array for deleting the rows that are to be moved around foreach ( $res as $row ) { - $usersAndGroups[] = [ 'ufg_user' => $row->ug_user, 'ufg_group' => $row->ug_group ]; + $insertData[] = [ 'ufg_user' => $row->ug_user, 'ufg_group' => $row->ug_group ]; + $deleteCond[] = $dbw->makeList( + [ 'ug_user' => $row->ug_user, 'ug_group' => $row->ug_group ], + $dbw::LIST_AND + ); } + // Delete the rows we're about to move + $dbw->delete( + 'user_groups', + $dbw->makeList( $deleteCond, $dbw::LIST_OR ), + __METHOD__ + ); + // Push the groups to user_former_groups + $dbw->insert( 'user_former_groups', $insertData, __METHOD__, [ 'IGNORE' ] ); + } - // delete 'em all - $dbw->delete( 'user_groups', $expiryCond, $fname ); + $dbw->endAtomic( __METHOD__ ); - // and push the groups to user_former_groups - $dbw->insert( 'user_former_groups', $usersAndGroups, __METHOD__, [ 'IGNORE' ] ); - } - ) ); + $lbFactory->commitAndWaitForReplication( __METHOD__, $ticket ); + } while ( $res->numRows() > 0 ); } /** diff --git a/tests/phpunit/includes/SiteStatsTest.php b/tests/phpunit/includes/SiteStatsTest.php index cdbf9fd944..56bde5da08 100644 --- a/tests/phpunit/includes/SiteStatsTest.php +++ b/tests/phpunit/includes/SiteStatsTest.php @@ -11,9 +11,10 @@ class SiteStatsTest extends MediaWikiTestCase { $cache = \MediaWiki\MediaWikiServices::getInstance()->getMainWANObjectCache(); $jobq = JobQueueGroup::singleton(); - // Delete EditPage jobs that might have been left behind by other tests + // Delete jobs that might have been left behind by other tests $jobq->get( 'htmlCacheUpdate' )->delete(); $jobq->get( 'recentChangesUpdate' )->delete(); + $jobq->get( 'userGroupExpiry' )->delete(); $cache->delete( $cache->makeKey( 'SiteStats', 'jobscount' ) ); $jobq->push( new NullJob( Title::newMainPage(), [] ) ); -- 2.20.1