Make updateCategoryCounts() have better lag checks
authorAaron Schulz <aschulz@wikimedia.org>
Wed, 19 Oct 2016 03:55:35 +0000 (20:55 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Thu, 20 Oct 2016 22:33:18 +0000 (15:33 -0700)
* Add the lag checks to LinksUpdate. Previously, only
  LinksDeletionUpdate had any such checks.
* Remove the transaction hook usage, since the only two callers are
  already lag/contention aware. Deferring them just makes the wait
  checks pointless and they might end up happening all at once.
* Also set the visibility on some neigboring methods.
* Clean up LinksUpdate $existing variables in passing. Instead of
  overriding the same variable, use a differently named variable
  to avoid mistakes.

Bug: T95501
Change-Id: I43e3af17399417cbf0ab4e5e7d1f2bd518fa7e90

includes/deferred/LinksUpdate.php
includes/page/WikiPage.php

index c7d378e..229a9a2 100644 (file)
@@ -205,64 +205,85 @@ class LinksUpdate extends DataUpdate implements EnqueueableDataUpdate {
 
        protected function doIncrementalUpdate() {
                # Page links
-               $existing = $this->getExistingLinks();
-               $this->linkDeletions = $this->getLinkDeletions( $existing );
-               $this->linkInsertions = $this->getLinkInsertions( $existing );
+               $existingPL = $this->getExistingLinks();
+               $this->linkDeletions = $this->getLinkDeletions( $existingPL );
+               $this->linkInsertions = $this->getLinkInsertions( $existingPL );
                $this->incrTableUpdate( 'pagelinks', 'pl', $this->linkDeletions, $this->linkInsertions );
 
                # Image links
-               $existing = $this->getExistingImages();
-               $imageDeletes = $this->getImageDeletions( $existing );
-               $this->incrTableUpdate( 'imagelinks', 'il', $imageDeletes,
-                       $this->getImageInsertions( $existing ) );
+               $existingIL = $this->getExistingImages();
+               $imageDeletes = $this->getImageDeletions( $existingIL );
+               $this->incrTableUpdate(
+                       'imagelinks',
+                       'il',
+                       $imageDeletes,
+                       $this->getImageInsertions( $existingIL ) );
 
                # Invalidate all image description pages which had links added or removed
-               $imageUpdates = $imageDeletes + array_diff_key( $this->mImages, $existing );
+               $imageUpdates = $imageDeletes + array_diff_key( $this->mImages, $existingIL );
                $this->invalidateImageDescriptions( $imageUpdates );
 
                # External links
-               $existing = $this->getExistingExternals();
-               $this->incrTableUpdate( 'externallinks', 'el', $this->getExternalDeletions( $existing ),
-                       $this->getExternalInsertions( $existing ) );
+               $existingEL = $this->getExistingExternals();
+               $this->incrTableUpdate(
+                       'externallinks',
+                       'el',
+                       $this->getExternalDeletions( $existingEL ),
+                       $this->getExternalInsertions( $existingEL ) );
 
                # Language links
-               $existing = $this->getExistingInterlangs();
-               $this->incrTableUpdate( 'langlinks', 'll', $this->getInterlangDeletions( $existing ),
-                       $this->getInterlangInsertions( $existing ) );
+               $existingLL = $this->getExistingInterlangs();
+               $this->incrTableUpdate(
+                       'langlinks',
+                       'll',
+                       $this->getInterlangDeletions( $existingLL ),
+                       $this->getInterlangInsertions( $existingLL ) );
 
                # Inline interwiki links
-               $existing = $this->getExistingInterwikis();
-               $this->incrTableUpdate( 'iwlinks', 'iwl', $this->getInterwikiDeletions( $existing ),
-                       $this->getInterwikiInsertions( $existing ) );
+               $existingIW = $this->getExistingInterwikis();
+               $this->incrTableUpdate(
+                       'iwlinks',
+                       'iwl',
+                       $this->getInterwikiDeletions( $existingIW ),
+                       $this->getInterwikiInsertions( $existingIW ) );
 
                # Template links
-               $existing = $this->getExistingTemplates();
-               $this->incrTableUpdate( 'templatelinks', 'tl', $this->getTemplateDeletions( $existing ),
-                       $this->getTemplateInsertions( $existing ) );
+               $existingTL = $this->getExistingTemplates();
+               $this->incrTableUpdate(
+                       'templatelinks',
+                       'tl',
+                       $this->getTemplateDeletions( $existingTL ),
+                       $this->getTemplateInsertions( $existingTL ) );
 
                # Category links
-               $existing = $this->getExistingCategories();
-               $categoryDeletes = $this->getCategoryDeletions( $existing );
-               $this->incrTableUpdate( 'categorylinks', 'cl', $categoryDeletes,
-                       $this->getCategoryInsertions( $existing ) );
-
-               # Invalidate all categories which were added, deleted or changed (set symmetric difference)
-               $categoryInserts = array_diff_assoc( $this->mCategories, $existing );
+               $existingCL = $this->getExistingCategories();
+               $categoryDeletes = $this->getCategoryDeletions( $existingCL );
+               $this->incrTableUpdate(
+                       'categorylinks',
+                       'cl',
+                       $categoryDeletes,
+                       $this->getCategoryInsertions( $existingCL ) );
+               $categoryInserts = array_diff_assoc( $this->mCategories, $existingCL );
                $categoryUpdates = $categoryInserts + $categoryDeletes;
-               $this->invalidateCategories( $categoryUpdates );
-               $this->updateCategoryCounts( $categoryInserts, $categoryDeletes );
 
                # Page properties
-               $existing = $this->getExistingProperties();
-               $this->propertyDeletions = $this->getPropertyDeletions( $existing );
-               $this->incrTableUpdate( 'page_props', 'pp', $this->propertyDeletions,
-                       $this->getPropertyInsertions( $existing ) );
+               $existingPP = $this->getExistingProperties();
+               $this->propertyDeletions = $this->getPropertyDeletions( $existingPP );
+               $this->incrTableUpdate(
+                       'page_props',
+                       'pp',
+                       $this->propertyDeletions,
+                       $this->getPropertyInsertions( $existingPP ) );
 
                # Invalidate the necessary pages
-               $this->propertyInsertions = array_diff_assoc( $this->mProperties, $existing );
+               $this->propertyInsertions = array_diff_assoc( $this->mProperties, $existingPP );
                $changed = $this->propertyDeletions + $this->propertyInsertions;
                $this->invalidateProperties( $changed );
 
+               # Invalidate all categories which were added, deleted or changed (set symmetric difference)
+               $this->invalidateCategories( $categoryUpdates );
+               $this->updateCategoryCounts( $categoryInserts, $categoryDeletes );
+
                # Refresh links of all pages including this page
                # This will be in a separate transaction
                if ( $this->mRecursive ) {
@@ -324,7 +345,7 @@ class LinksUpdate extends DataUpdate implements EnqueueableDataUpdate {
        /**
         * @param array $cats
         */
-       function invalidateCategories( $cats ) {
+       private function invalidateCategories( $cats ) {
                PurgeJobUtils::invalidatePages( $this->getDB(), NS_CATEGORY, array_keys( $cats ) );
        }
 
@@ -333,17 +354,31 @@ class LinksUpdate extends DataUpdate implements EnqueueableDataUpdate {
         * @param array $added Associative array of category name => sort key
         * @param array $deleted Associative array of category name => sort key
         */
-       function updateCategoryCounts( $added, $deleted ) {
-               $a = WikiPage::factory( $this->mTitle );
-               $a->updateCategoryCounts(
-                       array_keys( $added ), array_keys( $deleted )
-               );
+       private function updateCategoryCounts( array $added, array $deleted ) {
+               global $wgUpdateRowsPerQuery;
+
+               $wp = WikiPage::factory( $this->mTitle );
+               $factory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory();
+
+               foreach ( array_chunk( array_keys( $added ), $wgUpdateRowsPerQuery ) as $addBatch ) {
+                       $wp->updateCategoryCounts( $addBatch, [], $this->mId );
+                       $factory->commitAndWaitForReplication(
+                               __METHOD__, $this->ticket, [ 'wiki' => $this->getDB()->getWikiID() ]
+                       );
+               }
+
+               foreach ( array_chunk( array_keys( $deleted ), $wgUpdateRowsPerQuery ) as $deleteBatch ) {
+                       $wp->updateCategoryCounts( [], $deleteBatch, $this->mId );
+                       $factory->commitAndWaitForReplication(
+                               __METHOD__, $this->ticket, [ 'wiki' => $this->getDB()->getWikiID() ]
+                       );
+               }
        }
 
        /**
         * @param array $images
         */
-       function invalidateImageDescriptions( $images ) {
+       private function invalidateImageDescriptions( $images ) {
                PurgeJobUtils::invalidatePages( $this->getDB(), NS_FILE, array_keys( $images ) );
        }
 
index 3dc41fb..9aa8503 100644 (file)
@@ -3547,107 +3547,103 @@ class WikiPage implements Page, IDBAccessObject {
         * Update all the appropriate counts in the category table, given that
         * we've added the categories $added and deleted the categories $deleted.
         *
+        * This should only be called from deferred updates or jobs to avoid contention.
+        *
         * @param array $added The names of categories that were added
         * @param array $deleted The names of categories that were deleted
         * @param integer $id Page ID (this should be the original deleted page ID)
         */
        public function updateCategoryCounts( array $added, array $deleted, $id = 0 ) {
                $id = $id ?: $this->getId();
+               $ns = $this->getTitle()->getNamespace();
+
+               $addFields = [ 'cat_pages = cat_pages + 1' ];
+               $removeFields = [ 'cat_pages = cat_pages - 1' ];
+               if ( $ns == NS_CATEGORY ) {
+                       $addFields[] = 'cat_subcats = cat_subcats + 1';
+                       $removeFields[] = 'cat_subcats = cat_subcats - 1';
+               } elseif ( $ns == NS_FILE ) {
+                       $addFields[] = 'cat_files = cat_files + 1';
+                       $removeFields[] = 'cat_files = cat_files - 1';
+               }
+
                $dbw = wfGetDB( DB_MASTER );
-               $method = __METHOD__;
-               // Do this at the end of the commit to reduce lock wait timeouts
-               $dbw->onTransactionPreCommitOrIdle(
-                       function () use ( $dbw, $added, $deleted, $id, $method ) {
-                               $ns = $this->getTitle()->getNamespace();
-
-                               $addFields = [ 'cat_pages = cat_pages + 1' ];
-                               $removeFields = [ 'cat_pages = cat_pages - 1' ];
-                               if ( $ns == NS_CATEGORY ) {
-                                       $addFields[] = 'cat_subcats = cat_subcats + 1';
-                                       $removeFields[] = 'cat_subcats = cat_subcats - 1';
-                               } elseif ( $ns == NS_FILE ) {
-                                       $addFields[] = 'cat_files = cat_files + 1';
-                                       $removeFields[] = 'cat_files = cat_files - 1';
-                               }
 
-                               if ( count( $added ) ) {
-                                       $existingAdded = $dbw->selectFieldValues(
-                                               'category',
-                                               'cat_title',
-                                               [ 'cat_title' => $added ],
-                                               $method
-                                       );
+               if ( count( $added ) ) {
+                       $existingAdded = $dbw->selectFieldValues(
+                               'category',
+                               'cat_title',
+                               [ 'cat_title' => $added ],
+                               __METHOD__
+                       );
 
-                                       // For category rows that already exist, do a plain
-                                       // UPDATE instead of INSERT...ON DUPLICATE KEY UPDATE
-                                       // to avoid creating gaps in the cat_id sequence.
-                                       if ( count( $existingAdded ) ) {
-                                               $dbw->update(
-                                                       'category',
-                                                       $addFields,
-                                                       [ 'cat_title' => $existingAdded ],
-                                                       $method
-                                               );
-                                       }
+                       // For category rows that already exist, do a plain
+                       // UPDATE instead of INSERT...ON DUPLICATE KEY UPDATE
+                       // to avoid creating gaps in the cat_id sequence.
+                       if ( count( $existingAdded ) ) {
+                               $dbw->update(
+                                       'category',
+                                       $addFields,
+                                       [ 'cat_title' => $existingAdded ],
+                                       __METHOD__
+                               );
+                       }
 
-                                       $missingAdded = array_diff( $added, $existingAdded );
-                                       if ( count( $missingAdded ) ) {
-                                               $insertRows = [];
-                                               foreach ( $missingAdded as $cat ) {
-                                                       $insertRows[] = [
-                                                               'cat_title'   => $cat,
-                                                               'cat_pages'   => 1,
-                                                               'cat_subcats' => ( $ns == NS_CATEGORY ) ? 1 : 0,
-                                                               'cat_files'   => ( $ns == NS_FILE ) ? 1 : 0,
-                                                       ];
-                                               }
-                                               $dbw->upsert(
-                                                       'category',
-                                                       $insertRows,
-                                                       [ 'cat_title' ],
-                                                       $addFields,
-                                                       $method
-                                               );
-                                       }
+                       $missingAdded = array_diff( $added, $existingAdded );
+                       if ( count( $missingAdded ) ) {
+                               $insertRows = [];
+                               foreach ( $missingAdded as $cat ) {
+                                       $insertRows[] = [
+                                               'cat_title'   => $cat,
+                                               'cat_pages'   => 1,
+                                               'cat_subcats' => ( $ns == NS_CATEGORY ) ? 1 : 0,
+                                               'cat_files'   => ( $ns == NS_FILE ) ? 1 : 0,
+                                       ];
                                }
+                               $dbw->upsert(
+                                       'category',
+                                       $insertRows,
+                                       [ 'cat_title' ],
+                                       $addFields,
+                                       __METHOD__
+                               );
+                       }
+               }
 
-                               if ( count( $deleted ) ) {
-                                       $dbw->update(
-                                               'category',
-                                               $removeFields,
-                                               [ 'cat_title' => $deleted ],
-                                               $method
-                                       );
-                               }
+               if ( count( $deleted ) ) {
+                       $dbw->update(
+                               'category',
+                               $removeFields,
+                               [ 'cat_title' => $deleted ],
+                               __METHOD__
+                       );
+               }
 
-                               foreach ( $added as $catName ) {
-                                       $cat = Category::newFromName( $catName );
-                                       Hooks::run( 'CategoryAfterPageAdded', [ $cat, $this ] );
-                               }
+               foreach ( $added as $catName ) {
+                       $cat = Category::newFromName( $catName );
+                       Hooks::run( 'CategoryAfterPageAdded', [ $cat, $this ] );
+               }
 
-                               foreach ( $deleted as $catName ) {
-                                       $cat = Category::newFromName( $catName );
-                                       Hooks::run( 'CategoryAfterPageRemoved', [ $cat, $this, $id ] );
-                               }
+               foreach ( $deleted as $catName ) {
+                       $cat = Category::newFromName( $catName );
+                       Hooks::run( 'CategoryAfterPageRemoved', [ $cat, $this, $id ] );
+               }
 
-                               // Refresh counts on categories that should be empty now, to
-                               // trigger possible deletion. Check master for the most
-                               // up-to-date cat_pages.
-                               if ( count( $deleted ) ) {
-                                       $rows = $dbw->select(
-                                               'category',
-                                               [ 'cat_id', 'cat_title', 'cat_pages', 'cat_subcats', 'cat_files' ],
-                                               [ 'cat_title' => $deleted, 'cat_pages <= 0' ],
-                                               $method
-                                       );
-                                       foreach ( $rows as $row ) {
-                                               $cat = Category::newFromRow( $row );
-                                               $cat->refreshCounts();
-                                       }
-                               }
-                       },
-                       __METHOD__
-               );
+               // Refresh counts on categories that should be empty now, to
+               // trigger possible deletion. Check master for the most
+               // up-to-date cat_pages.
+               if ( count( $deleted ) ) {
+                       $rows = $dbw->select(
+                               'category',
+                               [ 'cat_id', 'cat_title', 'cat_pages', 'cat_subcats', 'cat_files' ],
+                               [ 'cat_title' => $deleted, 'cat_pages <= 0' ],
+                               __METHOD__
+                       );
+                       foreach ( $rows as $row ) {
+                               $cat = Category::newFromRow( $row );
+                               $cat->refreshCounts();
+                       }
+               }
        }
 
        /**