Merge "rdbms: add IDatabase::lockForUpdate() convenience method"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Wed, 11 Jul 2018 19:52:31 +0000 (19:52 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Wed, 11 Jul 2018 19:52:31 +0000 (19:52 +0000)
1  2 
includes/Category.php
includes/filerepo/file/LocalFile.php
includes/libs/rdbms/database/DBConnRef.php
includes/libs/rdbms/database/Database.php
includes/libs/rdbms/database/IDatabase.php
includes/page/WikiPage.php

diff --combined includes/Category.php
@@@ -171,7 -171,7 +171,7 @@@ class Category 
         *   fields are null, the resulting Category object will represent an empty
         *   category if a title object was given. If the fields are null and no
         *   title was given, this method fails and returns false.
 -       * @param Title $title Optional title object for the category represented by
 +       * @param Title|null $title Optional title object for the category represented by
         *   the given row. May be provided if it is already known, to avoid having
         *   to re-create a title object later.
         * @return Category|false
  
                // Lock the `category` row before locking `categorylinks` rows to try
                // to avoid deadlocks with LinksDeletionUpdate (T195397)
-               $dbw->selectField(
-                       'category',
-                       1,
-                       [ 'cat_title' => $this->mName ],
-                       __METHOD__,
-                       [ 'FOR UPDATE' ]
-               );
+               $dbw->lockForUpdate( 'category', [ 'cat_title' => $this->mName ], __METHOD__ );
  
                // Lock all the `categorylinks` records and gaps for this category;
                // this is a separate query due to postgres/oracle limitations
@@@ -223,7 -223,7 +223,7 @@@ class LocalFile extends File 
                        'img_minor_mime',
                        'img_user',
                        'img_user_text',
 -                      'img_actor' => $wgActorTableSchemaMigrationStage > MIGRATION_OLD ? 'img_actor' : null,
 +                      'img_actor' => $wgActorTableSchemaMigrationStage > MIGRATION_OLD ? 'img_actor' : 'NULL',
                        'img_timestamp',
                        'img_sha1',
                ] + MediaWikiServices::getInstance()->getCommentStore()->getFields( 'img_description' );
@@@ -3344,19 -3344,15 +3344,15 @@@ class LocalFileMoveBatch 
                $status = $repo->newGood();
                $dbw = $this->db;
  
-               $hasCurrent = $dbw->selectField(
+               $hasCurrent = $dbw->lockForUpdate(
                        'image',
-                       '1',
                        [ 'img_name' => $this->oldName ],
-                       __METHOD__,
-                       [ 'FOR UPDATE' ]
+                       __METHOD__
                );
-               $oldRowCount = $dbw->selectRowCount(
+               $oldRowCount = $dbw->lockForUpdate(
                        'oldimage',
-                       '*',
                        [ 'oi_name' => $this->oldName ],
-                       __METHOD__,
-                       [ 'FOR UPDATE' ]
+                       __METHOD__
                );
  
                if ( $hasCurrent ) {
@@@ -167,9 -167,6 +167,9 @@@ class DBConnRef implements IDatabase 
                return $this->__call( __FUNCTION__, func_get_args() );
        }
  
 +      /**
 +       * @codeCoverageIgnore
 +       */
        public function getWikiID() {
                return $this->getDomainID();
        }
                return $this->__call( __FUNCTION__, func_get_args() );
        }
  
+       public function lockForUpdate(
+               $table, $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
+       ) {
+               return $this->__call( __FUNCTION__, func_get_args() );
+       }
        public function fieldExists( $table, $field, $fname = __METHOD__ ) {
                return $this->__call( __FUNCTION__, func_get_args() );
        }
@@@ -1876,6 -1876,22 +1876,22 @@@ abstract class Database implements IDat
                return $column;
        }
  
+       public function lockForUpdate(
+               $table, $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
+       ) {
+               if ( !$this->trxLevel && !$this->getFlag( self::DBO_TRX ) ) {
+                       throw new DBUnexpectedError(
+                               $this,
+                               __METHOD__ . ': no transaction is active nor is DBO_TRX set'
+                       );
+               }
+               $options = (array)$options;
+               $options[] = 'FOR UPDATE';
+               return $this->selectRowCount( $table, '*', $conds, $fname, $options, $join_conds );
+       }
        /**
         * Removes most variables from an SQL query and replaces them with X or N for numbers.
         * It's only slightly flawed. Don't use for anything important.
                }
        }
  
 -      public function tableExists( $table, $fname = __METHOD__ ) {
 -              $tableRaw = $this->tableName( $table, 'raw' );
 -              if ( isset( $this->sessionTempTables[$tableRaw] ) ) {
 -                      return true; // already known to exist
 -              }
 -
 -              $table = $this->tableName( $table );
 -              $ignoreErrors = true;
 -              $res = $this->query( "SELECT 1 FROM $table LIMIT 1", $fname, $ignoreErrors );
 -
 -              return (bool)$res;
 -      }
 +      abstract public function tableExists( $table, $fname = __METHOD__ );
  
        public function indexUnique( $table, $index ) {
                $indexInfo = $this->indexInfo( $table, $index );
         * @see WANObjectCache::getWithSetCallback()
         *
         * @param IDatabase $db1
 -       * @param IDatabase $db2 [optional]
 +       * @param IDatabase|null $db2 [optional]
         * @return array Map of values:
         *   - lag: highest lag of any of the DBs or false on error (e.g. replication stopped)
         *   - since: oldest UNIX timestamp of any of the DB lag estimates
@@@ -166,14 -166,14 +166,14 @@@ interface IDatabase 
  
        /**
         * Get/set the table prefix.
 -       * @param string $prefix The table prefix to set, or omitted to leave it unchanged.
 +       * @param string|null $prefix The table prefix to set, or omitted to leave it unchanged.
         * @return string The previous table prefix.
         */
        public function tablePrefix( $prefix = null );
  
        /**
         * Get/set the db schema.
 -       * @param string $schema The database schema to set, or omitted to leave it unchanged.
 +       * @param string|null $schema The database schema to set, or omitted to leave it unchanged.
         * @return string The previous db schema.
         */
        public function dbSchema( $schema = null );
         * Get properties passed down from the server info array of the load
         * balancer.
         *
 -       * @param string $name The entry of the info array to get, or null to get the
 +       * @param string|null $name The entry of the info array to get, or null to get the
         *   whole array
         *
         * @return array|mixed|null
         * parameters, the member with the given name is set to the given value.
         *
         * @param string $name
 -       * @param array $value
 +       * @param array|null $value
         */
        public function setLBInfo( $name, $value = null );
  
                $tables, $var = '*', $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
        );
  
+       /**
+        * Lock all rows meeting the given conditions/options FOR UPDATE
+        *
+        * @param array|string $table Table names
+        * @param array|string $conds Filters on the table
+        * @param string $fname Function name for profiling
+        * @param array $options Options for select ("FOR UPDATE" is added automatically)
+        * @param array $join_conds Join conditions
+        * @return int Number of matching rows found (and locked)
+        * @since 1.32
+        */
+       public function lockForUpdate(
+               $table, $conds = '', $fname = __METHOD__, $options = [], $join_conds = []
+       );
        /**
         * Determines whether a field exists in a table
         *
         * @since 1.31
         * @see IDatabase::startAtomic
         * @param string $fname
 -       * @param AtomicSectionIdentifier $sectionId Section ID from startAtomic();
 +       * @param AtomicSectionIdentifier|null $sectionId Section ID from startAtomic();
         *   passing this enables cancellation of unclosed nested sections [optional]
         * @throws DBError
         */
         * The result is unquoted, and needs to be passed through addQuotes()
         * before it can be included in raw SQL.
         *
 -       * @param string|int $ts
 +       * @param string|int|null $ts
         *
         * @return string
         */
@@@ -768,7 -768,7 +768,7 @@@ class WikiPage implements Page, IDBAcce
         *   Revision::FOR_PUBLIC       to be displayed to all users
         *   Revision::FOR_THIS_USER    to be displayed to $wgUser
         *   Revision::RAW              get the text regardless of permissions
 -       * @param User $user User object to check for, only if FOR_THIS_USER is passed
 +       * @param User|null $user User object to check for, only if FOR_THIS_USER is passed
         *   to the $audience parameter
         * @return Content|null The content of the current revision
         *
         *   Revision::FOR_PUBLIC       to be displayed to all users
         *   Revision::FOR_THIS_USER    to be displayed to the given user
         *   Revision::RAW              get the text regardless of permissions
 -       * @param User $user User object to check for, only if FOR_THIS_USER is passed
 +       * @param User|null $user User object to check for, only if FOR_THIS_USER is passed
         *   to the $audience parameter
         * @return int User ID for the user that made the last article revision
         */
         *   Revision::FOR_PUBLIC       to be displayed to all users
         *   Revision::FOR_THIS_USER    to be displayed to the given user
         *   Revision::RAW              get the text regardless of permissions
 -       * @param User $user User object to check for, only if FOR_THIS_USER is passed
 +       * @param User|null $user User object to check for, only if FOR_THIS_USER is passed
         *   to the $audience parameter
         * @return User|null
         */
         *   Revision::FOR_PUBLIC       to be displayed to all users
         *   Revision::FOR_THIS_USER    to be displayed to the given user
         *   Revision::RAW              get the text regardless of permissions
 -       * @param User $user User object to check for, only if FOR_THIS_USER is passed
 +       * @param User|null $user User object to check for, only if FOR_THIS_USER is passed
         *   to the $audience parameter
         * @return string Username of the user that made the last article revision
         */
         *   Revision::FOR_PUBLIC       to be displayed to all users
         *   Revision::FOR_THIS_USER    to be displayed to the given user
         *   Revision::RAW              get the text regardless of permissions
 -       * @param User $user User object to check for, only if FOR_THIS_USER is passed
 +       * @param User|null $user User object to check for, only if FOR_THIS_USER is passed
         *   to the $audience parameter
         * @return string Comment stored for the last article revision
         */
         * @param IDatabase $dbw
         * @param Revision $revision For ID number, and text used to set
         *   length and redirect status fields
 -       * @param int $lastRevision If given, will not overwrite the page field
 +       * @param int|null $lastRevision If given, will not overwrite the page field
         *   when different from the currently set value.
         *   Giving 0 indicates the new page flag should be set on.
 -       * @param bool $lastRevIsRedirect If given, will optimize adding and
 +       * @param bool|null $lastRevIsRedirect If given, will optimize adding and
         *   removing rows in redirect table.
         * @return bool Success; false if the page row was missing or page_latest changed
         */
         *        to perform updates, if the edit was already saved.
         * @param RevisionSlotsUpdate|null $forUpdate The new content to be saved by the edit (pre PST),
         *        if the edit was not yet saved.
 +       * @param bool $forEdit Only re-use if the cached DerivedPageDataUpdater has the current
 +       *       revision as the edit's parent revision. This ensures that the same
 +       *       DerivedPageDataUpdater cannot be re-used for two consecutive edits.
         *
         * @return DerivedPageDataUpdater
         */
        private function getDerivedDataUpdater(
                User $forUser = null,
                RevisionRecord $forRevision = null,
 -              RevisionSlotsUpdate $forUpdate = null
 +              RevisionSlotsUpdate $forUpdate = null,
 +              $forEdit = false
        ) {
                if ( !$forRevision && !$forUpdate ) {
                        // NOTE: can't re-use an existing derivedDataUpdater if we don't know what the caller is
                        && !$this->derivedDataUpdater->isReusableFor(
                                $forUser,
                                $forRevision,
 -                              $forUpdate
 +                              $forUpdate,
 +                              $forEdit ? $this->getLatest() : null
                        )
                ) {
                        $this->derivedDataUpdater = null;
         * @since 1.32
         *
         * @param User $user
 +       * @param RevisionSlotsUpdate|null $forUpdate If given, allows any cached ParserOutput
 +       *        that may already have been returned via getDerivedDataUpdater to be re-used.
         *
         * @return PageUpdater
         */
 -      public function newPageUpdater( User $user ) {
 +      public function newPageUpdater( User $user, RevisionSlotsUpdate $forUpdate = null ) {
                global $wgAjaxEditStash, $wgUseAutomaticEditSummaries, $wgPageCreationLog;
  
                $pageUpdater = new PageUpdater(
                        $user,
                        $this, // NOTE: eventually, PageUpdater should not know about WikiPage
 -                      $this->getDerivedDataUpdater( $user ),
 +                      $this->getDerivedDataUpdater( $user, null, $forUpdate, true ),
                        $this->getDBLoadBalancer(),
                        $this->getRevisionStore()
                );
         * restores or repeats. The new revision is expected to have the exact same content as
         * the given original revision. This is used with rollbacks and with dummy "null" revisions
         * which are created to record things like page moves.
 -       * @param User $user The user doing the edit
 -       * @param string $serialFormat IGNORED.
 +       * @param User|null $user The user doing the edit
 +       * @param string|null $serialFormat IGNORED.
         * @param array|null $tags Change tags to apply to this edit
         * Callers are responsible for permission checks
         * (with ChangeTags::canAddTagsAccompanyingChange)
                        $flags = ( $flags & ~EDIT_MINOR );
                }
  
 +              $slotsUpdate = new RevisionSlotsUpdate();
 +              $slotsUpdate->modifyContent( 'main', $content );
 +
                // NOTE: while doEditContent() executes, callbacks to getDerivedDataUpdater and
                // prepareContentForEdit will generally use the DerivedPageDataUpdater that is also
                // used by this PageUpdater. However, there is no guarantee for this.
 -              $updater = $this->newPageUpdater( $user );
 +              $updater = $this->newPageUpdater( $user, $slotsUpdate );
                $updater->setContent( 'main', $content );
                $updater->setOriginalRevisionId( $originalRevId );
                $updater->setUndidRevisionId( $undidRevId );
         * @param int &$cascade Set to false if cascading protection isn't allowed.
         * @param string $reason
         * @param User $user The user updating the restrictions
 -       * @param string|string[] $tags Change tags to add to the pages and protection log entries
 +       * @param string|string[]|null $tags Change tags to add to the pages and protection log entries
         *   ($user should be able to add the specified tags before this is called)
         * @return Status Status object; if action is taken, $status->value is the log_id of the
         *   protection log entry.
         * @param string $reason Delete reason for deletion log
         * @param bool $suppress Suppress all revisions and log the deletion in
         *        the suppression log instead of the deletion log
 -       * @param int $u1 Unused
 -       * @param bool $u2 Unused
 +       * @param int|null $u1 Unused
 +       * @param bool|null $u2 Unused
         * @param array|string &$error Array of errors to append to
 -       * @param User $user The deleting user
 +       * @param User|null $user The deleting user
         * @return bool True if successful
         */
        public function doDeleteArticle(
         * @param string $reason Delete reason for deletion log
         * @param bool $suppress Suppress all revisions and log the deletion in
         *   the suppression log instead of the deletion log
 -       * @param int $u1 Unused
 -       * @param bool $u2 Unused
 +       * @param int|null $u1 Unused
 +       * @param bool|null $u2 Unused
         * @param array|string &$error Array of errors to append to
 -       * @param User $deleter The deleting user
 +       * @param User|null $deleter The deleting user
         * @param array $tags Tags to apply to the deletion action
         * @param string $logsubtype
         * @return Status Status object; if successful, $status->value is the log_id of the
                // Note array_intersect() preserves keys from the first arg, and we're
                // assuming $revQuery has `revision` primary and isn't using subtables
                // for anything we care about.
-               $res = $dbw->select(
+               $dbw->lockForUpdate(
                        array_intersect(
                                $revQuery['tables'],
                                [ 'revision', 'revision_comment_temp', 'revision_actor_temp' ]
                        ),
-                       '1',
                        [ 'rev_page' => $id ],
                        __METHOD__,
-                       'FOR UPDATE',
+                       [],
                        $revQuery['joins']
                );
-               foreach ( $res as $row ) {
-                       // Fetch all rows in case the DB needs that to properly lock them.
-               }
  
 -              // Get all of the page revisions
 +              // If SCHEMA_COMPAT_WRITE_OLD is set, also select all extra fields we still write,
 +              // so we can copy it to the archive table.
 +              // We know the fields exist, otherwise SCHEMA_COMPAT_WRITE_OLD could not function.
 +              if ( $wgMultiContentRevisionSchemaMigrationStage & SCHEMA_COMPAT_WRITE_OLD ) {
 +                      $revQuery['fields'][] = 'rev_text_id';
 +
 +                      if ( $wgContentHandlerUseDB ) {
 +                              $revQuery['fields'][] = 'rev_content_model';
 +                              $revQuery['fields'][] = 'rev_content_format';
 +                      }
 +              }
 +
 +                      // Get all of the page revisions
                $res = $dbw->select(
                        $revQuery['tables'],
                        $revQuery['fields'],
                        ] + $commentStore->insert( $dbw, 'ar_comment', $comment )
                                + $actorMigration->getInsertValues( $dbw, 'ar_user', $user );
  
 -                      if ( $wgMultiContentRevisionSchemaMigrationStage < MIGRATION_NEW ) {
 +                      if ( $wgMultiContentRevisionSchemaMigrationStage & SCHEMA_COMPAT_WRITE_OLD ) {
                                $rowInsert['ar_text_id'] = $row->rev_text_id;
 -                      }
  
 -                      if (
 -                              $wgContentHandlerUseDB &&
 -                              $wgMultiContentRevisionSchemaMigrationStage <= MIGRATION_WRITE_BOTH
 -                      ) {
 -                              $rowInsert['ar_content_model'] = $row->rev_content_model;
 -                              $rowInsert['ar_content_format'] = $row->rev_content_format;
 +                              if ( $wgContentHandlerUseDB ) {
 +                                      $rowInsert['ar_content_model'] = $row->rev_content_model;
 +                                      $rowInsert['ar_content_format'] = $row->rev_content_format;
 +                              }
                        }
 +
                        $rowsInsert[] = $rowInsert;
                        $revids[] = $row->rev_id;