From 9eff263e8eb809f41d18428800f192d3fef5d958 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Mon, 2 Jul 2018 19:02:54 +0100 Subject: [PATCH] rdbms: add IDatabase::lockForUpdate() convenience method Change-Id: I238fd96407e1122e90058e2c4acf743044a267ec --- includes/Category.php | 8 +-- includes/MergeHistory.php | 12 +++-- includes/filerepo/file/LocalFile.php | 12 ++--- includes/libs/rdbms/database/DBConnRef.php | 6 +++ includes/libs/rdbms/database/Database.php | 16 ++++++ includes/libs/rdbms/database/IDatabase.php | 15 ++++++ includes/page/WikiPage.php | 8 +-- .../libs/rdbms/database/DatabaseSQLTest.php | 52 +++++++++++++++++++ 8 files changed, 104 insertions(+), 25 deletions(-) diff --git a/includes/Category.php b/includes/Category.php index 41ecc65032..839f2dbbf4 100644 --- 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 4c655ebab5..0914a9b7d8 100644 --- 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 e86f292703..ba54514c71 100644 --- 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 b414a2a02a..0aec0c1db4 100644 --- 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 57e5907a52..c43149c5e0 100644 --- 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 21451292bb..a2eb1e6b06 100644 --- 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 261c9548e4..5608791620 100644 --- 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 d9919e15a4..0cb35b49c2 100644 --- 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 -- 2.20.1