Watchlist: Commit after each batch watchlist insertion and removal

With this change, adding large numbers (500+) of items to one's Watchlist via
Special:EditWatchlist/raw will no longer trigger DBPerformance warnings
for "max affected rows expectation not met". The same code mechanism is added
for bulk removal of watchlist items.

Bug: T171898
Depends-On: Ia0f496b8bfb2b68217d0f45f892045538494bfdc
Change-Id: I832b1843d1341b05227cdee2549bdcefa21eb300

diff --git a/includes/specials/SpecialEditWatchlist.php b/includes/specials/SpecialEditWatchlist.php
index 083b3c0..16cebe0 100644 (file)
--- a/includes/specials/SpecialEditWatchlist.php
+++ b/includes/specials/SpecialEditWatchlist.php
@@ -459,8 +459,58 @@ class SpecialEditWatchlist extends UnlistedSpecialPage {
         * Add a list of targets to a user's watchlist
         *
         * @param string[]|LinkTarget[] $targets
+        * @return bool
+        * @throws FatalError
+        * @throws MWException
         */
-       private function watchTitles( $targets ) {
+       private function watchTitles( array $targets ) {
+               return MediaWikiServices::getInstance()->getWatchedItemStore()
+                       ->addWatchBatchForUser( $this->getUser(), $this->getExpandedTargets( $targets ) )
+                       && $this->runWatchUnwatchCompleteHook( 'Watch', $targets );
+       }
+
+       /**
+        * Remove a list of titles from a user's watchlist
+        *
+        * $titles can be an array of strings or Title objects; the former
+        * is preferred, since Titles are very memory-heavy
+        *
+        * @param string[]|LinkTarget[] $targets
+        *
+        * @return bool
+        * @throws FatalError
+        * @throws MWException
+        */
+       private function unwatchTitles( array $targets ) {
+               return MediaWikiServices::getInstance()->getWatchedItemStore()
+                       ->removeWatchBatchForUser( $this->getUser(), $this->getExpandedTargets( $targets ) )
+                       && $this->runWatchUnwatchCompleteHook( 'Unwatch', $targets );
+       }
+
+       /**
+        * @param string $action
+        *   Can be "Watch" or "Unwatch"
+        * @param string[]|LinkTarget[] $targets
+        * @return bool
+        * @throws FatalError
+        * @throws MWException
+        */
+       private function runWatchUnwatchCompleteHook( $action, $targets ) {
+               foreach ( $targets as $target ) {
+                       $title = $target instanceof TitleValue ?
+                               Title::newFromTitleValue( $target ) :
+                               Title::newFromText( $target );
+                       $page = WikiPage::factory( $title );
+                       Hooks::run( $action . 'ArticleComplete', [ $this->getUser(), &$page ] );
+               }
+               return true;
+       }
+
+       /**
+        * @param string[]|LinkTarget[] $targets
+        * @return TitleValue[]
+        */
+       private function getExpandedTargets( array $targets ) {
                $expandedTargets = [];
                foreach ( $targets as $target ) {
                        if ( !$target instanceof LinkTarget ) {
@@ -477,37 +527,7 @@ class SpecialEditWatchlist extends UnlistedSpecialPage {
                        $expandedTargets[] = new TitleValue( MWNamespace::getSubject( $ns ), $dbKey );
                        $expandedTargets[] = new TitleValue( MWNamespace::getTalk( $ns ), $dbKey );
                }
-
-               MediaWikiServices::getInstance()->getWatchedItemStore()->addWatchBatchForUser(
-                       $this->getUser(),
-                       $expandedTargets
-               );
-       }
-
-       /**
-        * Remove a list of titles from a user's watchlist
-        *
-        * $titles can be an array of strings or Title objects; the former
-        * is preferred, since Titles are very memory-heavy
-        *
-        * @param array $titles Array of strings, or Title objects
-        */
-       private function unwatchTitles( $titles ) {
-               $store = MediaWikiServices::getInstance()->getWatchedItemStore();
-
-               foreach ( $titles as $title ) {
-                       if ( !$title instanceof Title ) {
-                               $title = Title::newFromText( $title );
-                       }
-
-                       if ( $title instanceof Title ) {
-                               $store->removeWatch( $this->getUser(), $title->getSubjectPage() );
-                               $store->removeWatch( $this->getUser(), $title->getTalkPage() );
-
-                               $page = WikiPage::factory( $title );
-                               Hooks::run( 'UnwatchArticleComplete', [ $this->getUser(), &$page ] );
-                       }
-               }
+               return $expandedTargets;
        }
 
        public function submitNormal( $data ) {

diff --git a/includes/watcheditem/NoWriteWatchedItemStore.php b/includes/watcheditem/NoWriteWatchedItemStore.php
index 86e7be8..f4e3af2 100644 (file)
--- a/includes/watcheditem/NoWriteWatchedItemStore.php
+++ b/includes/watcheditem/NoWriteWatchedItemStore.php
@@ -142,4 +142,9 @@ class NoWriteWatchedItemStore implements WatchedItemStoreInterface {
        public function clearUserWatchedItemsUsingJobQueue( User $user ) {
                throw new DBReadOnlyError( null, 'The watchlist is currently readonly.' );
        }
+
+       public function removeWatchBatchForUser( User $user, array $titles ) {
+               throw new DBReadOnlyError( null, 'The watchlist is currently readonly.' );
+       }
+
 }

diff --git a/includes/watcheditem/WatchedItemStore.php b/includes/watcheditem/WatchedItemStore.php
index c763010..f9435a1 100644 (file)
--- a/includes/watcheditem/WatchedItemStore.php
+++ b/includes/watcheditem/WatchedItemStore.php
@@ -4,6 +4,7 @@ use Wikimedia\Rdbms\IDatabase;
 use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface;
 use MediaWiki\Linker\LinkTarget;
 use Wikimedia\Assert\Assert;
+use Wikimedia\Rdbms\LBFactory;
 use Wikimedia\ScopedCallback;
 use Wikimedia\Rdbms\ILBFactory;
 use Wikimedia\Rdbms\LoadBalancer;
@@ -367,6 +368,47 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac
                return $visitingWatchers;
        }
 
+       /**
+        * @param User $user
+        * @param TitleValue[] $titles
+        * @return bool
+        * @throws MWException
+        */
+       public function removeWatchBatchForUser( User $user, array $titles ) {
+               if ( $this->readOnlyMode->isReadOnly() ) {
+                       return false;
+               }
+               if ( $user->isAnon() ) {
+                       return false;
+               }
+               if ( !$titles ) {
+                       return true;
+               }
+
+               $rows = $this->getTitleDbKeysGroupedByNamespace( $titles );
+               $this->uncacheTitlesForUser( $user, $titles );
+
+               $dbw = $this->getConnectionRef( DB_MASTER );
+               $ticket = $this->lbFactory->getEmptyTransactionTicket( __METHOD__ );
+               $affectedRows = 0;
+
+               // Batch delete items per namespace.
+               foreach ( $rows as $namespace => $namespaceTitles ) {
+                       $rowBatches = array_chunk( $namespaceTitles, $this->updateRowsPerQuery );
+                       foreach ( $rowBatches as $toDelete ) {
+                               $dbw->delete( 'watchlist', [
+                                       'wl_user' => $user->getId(),
+                                       'wl_namespace' => $namespace,
+                                       'wl_title' => $toDelete
+                               ], __METHOD__ );
+                               $affectedRows += $dbw->affectedRows();
+                               $this->lbFactory->commitAndWaitForReplication( __METHOD__, $ticket );
+                       }
+               }
+
+               return (bool)$affectedRows;
+       }
+
        /**
         * @since 1.27
         * @param LinkTarget[] $targets
@@ -655,6 +697,7 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac
         * @since 1.27
         * @param User $user
         * @param LinkTarget $target
+        * @throws MWException
         */
        public function addWatch( User $user, LinkTarget $target ) {
                $this->addWatchBatchForUser( $user, [ $target ] );
@@ -665,6 +708,7 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac
         * @param User $user
         * @param LinkTarget[] $targets
         * @return bool
+        * @throws MWException
         */
        public function addWatchBatchForUser( User $user, array $targets ) {
                if ( $this->readOnlyMode->isReadOnly() ) {
@@ -697,10 +741,15 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac
                }
 
                $dbw = $this->getConnectionRef( DB_MASTER );
-               foreach ( array_chunk( $rows, 100 ) as $toInsert ) {
+               $ticket = $this->lbFactory->getEmptyTransactionTicket( __METHOD__ );
+               $affectedRows = 0;
+               $rowBatches = array_chunk( $rows, $this->updateRowsPerQuery );
+               foreach ( $rowBatches as $toInsert ) {
                        // Use INSERT IGNORE to avoid overwriting the notification timestamp
                        // if there's already an entry for this page
                        $dbw->insert( 'watchlist', $toInsert, __METHOD__, 'IGNORE' );
+                       $affectedRows += $dbw->affectedRows();
+                       $this->lbFactory->commitAndWaitForReplication( __METHOD__, $ticket );
                }
                // Update process cache to ensure skin doesn't claim that the current
                // page is unwatched in the response of action=watch itself (T28292).
@@ -709,7 +758,7 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac
                        $this->cache( $item );
                }
 
-               return true;
+               return (bool)$affectedRows;
        }
 
        /**
@@ -717,26 +766,10 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac
         * @param User $user
         * @param LinkTarget $target
         * @return bool
+        * @throws MWException
         */
        public function removeWatch( User $user, LinkTarget $target ) {
-               // Only logged in user can have a watchlist
-               if ( $this->readOnlyMode->isReadOnly() || $user->isAnon() ) {
-                       return false;
-               }
-
-               $this->uncache( $user, $target );
-
-               $dbw = $this->getConnectionRef( DB_MASTER );
-               $dbw->delete( 'watchlist',
-                       [
-                               'wl_user' => $user->getId(),
-                               'wl_namespace' => $target->getNamespace(),
-                               'wl_title' => $target->getDBkey(),
-                       ], __METHOD__
-               );
-               $success = (bool)$dbw->affectedRows();
-
-               return $success;
+               return $this->removeWatchBatchForUser( $user, [ $target ] );
        }
 
        /**
@@ -1044,4 +1077,27 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac
                }
        }
 
+       /**
+        * @param TitleValue[] $titles
+        * @return array
+        */
+       private function getTitleDbKeysGroupedByNamespace( array $titles ) {
+               $rows = [];
+               foreach ( $titles as $title ) {
+                       // Group titles by namespace.
+                       $rows[ $title->getNamespace() ][] = $title->getDBkey();
+               }
+               return $rows;
+       }
+
+       /**
+        * @param User $user
+        * @param Title[] $titles
+        */
+       private function uncacheTitlesForUser( User $user, array $titles ) {
+               foreach ( $titles as $title ) {
+                       $this->uncache( $user, $title );
+               }
+       }
+
 }

diff --git a/includes/watcheditem/WatchedItemStoreInterface.php b/includes/watcheditem/WatchedItemStoreInterface.php
index 99a051d..ac5e215 100644 (file)
--- a/includes/watcheditem/WatchedItemStoreInterface.php
+++ b/includes/watcheditem/WatchedItemStoreInterface.php
@@ -316,4 +316,14 @@ interface WatchedItemStoreInterface {
         */
        public function clearUserWatchedItemsUsingJobQueue( User $user );
 
+       /**
+        * @since 1.32
+        *
+        * @param User $user
+        * @param LinkTarget[] $targets
+        *
+        * @return bool success
+        */
+       public function removeWatchBatchForUser( User $user, array $targets );
+
 }

diff --git a/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php b/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php
index f60f92c..240b3f5 100644 (file)
--- a/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php
+++ b/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php
@@ -1,7 +1,7 @@
 <?php
 use MediaWiki\Linker\LinkTarget;
-use Wikimedia\Rdbms\LoadBalancer;
 use Wikimedia\Rdbms\LBFactory;
+use Wikimedia\Rdbms\LoadBalancer;
 use Wikimedia\ScopedCallback;
 use Wikimedia\TestingAccessWrapper;
 
@@ -1106,6 +1106,10 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase {
                                ]
                        );
 
+               $mockDb->expects( $this->once() )
+                       ->method( 'affectedRows' )
+                       ->willReturn( 2 );
+
                $mockCache = $this->getMockCache();
                $mockCache->expects( $this->exactly( 2 ) )
                        ->method( 'delete' );
@@ -1276,23 +1280,36 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase {
                $mockDb = $this->getMockDb();
                $mockDb->expects( $this->once() )
                        ->method( 'delete' )
-                       ->with(
-                               'watchlist',
+                       ->withConsecutive(
                                [
-                                       'wl_user' => 1,
-                                       'wl_namespace' => 0,
-                                       'wl_title' => 'SomeDbKey',
+                                       'watchlist',
+                                       [
+                                               'wl_user' => 1,
+                                               'wl_namespace' => 0,
+                                               'wl_title' => [ 'SomeDbKey' ],
+                                       ],
+                               ],
+                               [
+                                       'watchlist',
+                                       [
+                                               'wl_user' => 1,
+                                               'wl_namespace' => 1,
+                                               'wl_title' => [ 'SomeDbKey' ],
+                                       ]
                                ]
                        );
-               $mockDb->expects( $this->once() )
+               $mockDb->expects( $this->exactly( 1 ) )
                        ->method( 'affectedRows' )
-                       ->will( $this->returnValue( 1 ) );
+                       ->willReturn( 2 );
 
                $mockCache = $this->getMockCache();
                $mockCache->expects( $this->never() )->method( 'get' );
                $mockCache->expects( $this->once() )
                        ->method( 'delete' )
-                       ->with( '0:SomeDbKey:1' );
+                       ->withConsecutive(
+                               [ '0:SomeDbKey:1' ],
+                               [ '1:SomeDbKey:1' ]
+                       );
 
                $store = $this->newWatchedItemStore(
                        $this->getMockLBFactory( $mockDb ),
@@ -1300,10 +1317,11 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase {
                        $this->getMockReadOnlyMode()
                );
 
+               $titleValue = new TitleValue( 0, 'SomeDbKey' );
                $this->assertTrue(
                        $store->removeWatch(
                                $this->getMockNonAnonUserWithId( 1 ),
-                               new TitleValue( 0, 'SomeDbKey' )
+                               Title::newFromTitleValue( $titleValue )
                        )
                );
        }
@@ -1312,23 +1330,37 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase {
                $mockDb = $this->getMockDb();
                $mockDb->expects( $this->once() )
                        ->method( 'delete' )
-                       ->with(
-                               'watchlist',
+                       ->withConsecutive(
                                [
-                                       'wl_user' => 1,
-                                       'wl_namespace' => 0,
-                                       'wl_title' => 'SomeDbKey',
+                                       'watchlist',
+                                       [
+                                               'wl_user' => 1,
+                                               'wl_namespace' => 0,
+                                               'wl_title' => [ 'SomeDbKey' ],
+                                       ]
+                               ],
+                               [
+                                       'watchlist',
+                                       [
+                                               'wl_user' => 1,
+                                               'wl_namespace' => 1,
+                                               'wl_title' => [ 'SomeDbKey' ],
+                                       ]
                                ]
                        );
+
                $mockDb->expects( $this->once() )
                        ->method( 'affectedRows' )
-                       ->will( $this->returnValue( 0 ) );
+                       ->willReturn( 0 );
 
                $mockCache = $this->getMockCache();
                $mockCache->expects( $this->never() )->method( 'get' );
                $mockCache->expects( $this->once() )
                        ->method( 'delete' )
-                       ->with( '0:SomeDbKey:1' );
+                       ->withConsecutive(
+                               [ '0:SomeDbKey:1' ],
+                               [ '1:SomeDbKey:1' ]
+                       );
 
                $store = $this->newWatchedItemStore(
                        $this->getMockLBFactory( $mockDb ),
@@ -1336,10 +1368,11 @@ class WatchedItemStoreUnitTest extends MediaWikiTestCase {
                        $this->getMockReadOnlyMode()
                );
 
+               $titleValue = new TitleValue( 0, 'SomeDbKey' );
                $this->assertFalse(
                        $store->removeWatch(
                                $this->getMockNonAnonUserWithId( 1 ),
-                               new TitleValue( 0, 'SomeDbKey' )
+                               Title::newFromTitleValue( $titleValue )
                        )
                );
        }