From 05b490bc4a1fa001ef1aa08e0dc960a841897a45 Mon Sep 17 00:00:00 2001 From: Kosta Harlan Date: Tue, 2 Oct 2018 15:55:53 -0400 Subject: [PATCH] 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 --- includes/specials/SpecialEditWatchlist.php | 84 +++++++++------- .../watcheditem/NoWriteWatchedItemStore.php | 5 + includes/watcheditem/WatchedItemStore.php | 96 +++++++++++++++---- .../watcheditem/WatchedItemStoreInterface.php | 10 ++ .../watcheditem/WatchedItemStoreUnitTest.php | 69 +++++++++---- 5 files changed, 194 insertions(+), 70 deletions(-) diff --git a/includes/specials/SpecialEditWatchlist.php b/includes/specials/SpecialEditWatchlist.php index 083b3c0d0f..16cebe0fae 100644 --- 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 86e7be855e..f4e3af2370 100644 --- 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 c76301046c..f9435a1b0e 100644 --- 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 99a051de9f..ac5e2159b5 100644 --- 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 f60f92ceb2..240b3f520f 100644 --- a/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php +++ b/tests/phpunit/includes/watcheditem/WatchedItemStoreUnitTest.php @@ -1,7 +1,7 @@ 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 ) ) ); } -- 2.20.1