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

diff --git a/autoload.php b/autoload.php
index fc77fcb..9042f7b 100644 (file)
--- 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 a6a3686..772e076 100644 (file)
--- 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 (file)
index 0000000..0945e58
--- /dev/null
+++ b/includes/jobqueue/jobs/UserGroupExpiryJob.php
@@ -0,0 +1,39 @@
+<?php
+/**
+ * Job that purges expired user group memberships.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ * @ingroup JobQueue
+ */
+
+class UserGroupExpiryJob extends Job {
+       public function __construct( $params = false ) {
+               parent::__construct( 'userGroupExpiry', Title::newMainPage(), $params );
+               $this->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 f771f42..e757e59 100644 (file)
--- 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 cdbf9fd..56bde5d 100644 (file)
--- 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(), [] ) );