rdbms: add IDatabase::lockForUpdate() convenience method

Change-Id: I238fd96407e1122e90058e2c4acf743044a267ec

diff --git a/includes/Category.php b/includes/Category.php
index 41ecc65..839f2db 100644 (file)
--- a/includes/Category.php
+++ b/includes/Category.php
@@ -337,13 +337,7 @@ class Category {
 
                // 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

diff --git a/includes/MergeHistory.php b/includes/MergeHistory.php
index 4c655eb..0914a9b 100644 (file)
--- a/includes/MergeHistory.php
+++ b/includes/MergeHistory.php
@@ -254,6 +254,8 @@ class MergeHistory {
                        return $permCheck;
                }
 
+               $this->dbw->startAtomic( __METHOD__ );
+
                $this->dbw->update(
                        'revision',
                        [ 'rev_page' => $this->dest->getArticleID() ],
@@ -264,17 +266,17 @@ class MergeHistory {
                // Check if this did anything
                $this->revisionsMerged = $this->dbw->affectedRows();
                if ( $this->revisionsMerged < 1 ) {
+                       $this->dbw->endAtomic( __METHOD__ );
                        $status->fatal( 'mergehistory-fail-no-change' );
+
                        return $status;
                }
 
                // Make the source page a redirect if no revisions are left
-               $haveRevisions = $this->dbw->selectField(
+               $haveRevisions = $this->dbw->lockForUpdate(
                        'revision',
-                       'rev_timestamp',
                        [ 'rev_page' => $this->source->getArticleID() ],
-                       __METHOD__,
-                       [ 'FOR UPDATE' ]
+                       __METHOD__
                );
                if ( !$haveRevisions ) {
                        if ( $reason ) {
@@ -350,6 +352,8 @@ class MergeHistory {
 
                Hooks::run( 'ArticleMergeComplete', [ $this->source, $this->dest ] );
 
+               $this->dbw->endAtomic( __METHOD__ );
+
                return $status;
        }
 }

diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index e86f292..ba54514 100644 (file)
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -3344,19 +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 ) {

diff --git a/includes/libs/rdbms/database/DBConnRef.php b/includes/libs/rdbms/database/DBConnRef.php
index b414a2a..0aec0c1 100644 (file)
--- a/includes/libs/rdbms/database/DBConnRef.php
+++ b/includes/libs/rdbms/database/DBConnRef.php
@@ -284,6 +284,12 @@ class DBConnRef implements IDatabase {
                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() );
        }

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 57e5907..c43149c 100644 (file)
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -1876,6 +1876,22 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                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.

diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php
index 2145129..a2eb1e6 100644 (file)
--- a/includes/libs/rdbms/database/IDatabase.php
+++ b/includes/libs/rdbms/database/IDatabase.php
@@ -848,6 +848,21 @@ interface IDatabase {
                $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
         *

diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php
index 261c954..5608791 100644 (file)
--- a/includes/page/WikiPage.php
+++ b/includes/page/WikiPage.php
@@ -2527,20 +2527,16 @@ class WikiPage implements Page, IDBAccessObject {
                // 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
                $res = $dbw->select(

diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
index d9919e1..0cb35b4 100644 (file)
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php
@@ -256,6 +256,58 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase {
                ];
        }
 
+       /**
+        * @dataProvider provideLockForUpdate
+        * @covers Wikimedia\Rdbms\Database::lockForUpdate
+        */
+       public function testLockForUpdate( $sql, $sqlText ) {
+               $this->database->startAtomic( __METHOD__ );
+               $this->database->lockForUpdate(
+                       $sql['tables'],
+                       $sql['conds'] ?? [],
+                       __METHOD__,
+                       $sql['options'] ?? [],
+                       $sql['join_conds'] ?? []
+               );
+               $this->database->endAtomic( __METHOD__ );
+
+               $this->assertLastSql( "BEGIN; $sqlText; COMMIT" );
+       }
+
+       public static function provideLockForUpdate() {
+               return [
+                       [
+                               [
+                                       'tables' => [ 'table' ],
+                                       'conds' => [ 'field' => [ 1, 2, 3, 4 ] ],
+                               ],
+                               "SELECT COUNT(*) AS rowcount FROM " .
+                               "(SELECT 1 FROM table WHERE field IN ('1','2','3','4')    " .
+                               "FOR UPDATE) tmp_count"
+                       ],
+                       [
+                               [
+                                       'tables' => [ 'table', 't2' => 'table2' ],
+                                       'conds' => [ 'field' => 'text' ],
+                                       'options' => [ 'LIMIT' => 1, 'ORDER BY' => 'field' ],
+                                       'join_conds' => [ 't2' => [
+                                               'LEFT JOIN', 'tid = t2.id'
+                                       ] ],
+                               ],
+                               "SELECT COUNT(*) AS rowcount FROM " .
+                               "(SELECT 1 FROM table LEFT JOIN table2 t2 ON ((tid = t2.id)) " .
+                               "WHERE field = 'text' ORDER BY field LIMIT 1   FOR UPDATE) tmp_count"
+                       ],
+                       [
+                               [
+                                       'tables' => 'table',
+                               ],
+                               "SELECT COUNT(*) AS rowcount FROM " .
+                               "(SELECT 1 FROM table      FOR UPDATE) tmp_count"
+                       ],
+               ];
+       }
+
        /**
         * @covers Wikimedia\Rdbms\Subquery
         * @dataProvider provideSelectRowCount