From c9ad7037cef3b0a96b2b0e22e21eb7d1e7204d42 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Wed, 14 Feb 2018 00:27:14 -0800 Subject: [PATCH] rdbms: do not bother making DBO_TRX transactions in IDatabase::lock() Named locks are session-level constructs and this transaction agnostic. Also make lockIsFree() a bit more consistent when the thread has the lock itself. Change-Id: Ief51196161bbc50c798740f3c738fd0e39880508 --- includes/libs/rdbms/database/Database.php | 5 +++- .../libs/rdbms/database/DatabaseMysqlBase.php | 9 +++++++ .../libs/rdbms/database/DatabasePostgres.php | 8 +++++++ includes/libs/rdbms/database/IDatabase.php | 2 +- .../libs/rdbms/database/DatabaseTest.php | 24 +++++++++++++++++++ 5 files changed, 46 insertions(+), 2 deletions(-) diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index c6c7a9b7cf..9a8996c7b3 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -3561,7 +3561,10 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } public function lockIsFree( $lockName, $method ) { - return true; + // RDBMs methods for checking named locks may or may not count this thread itself. + // In MySQL, IS_FREE_LOCK() returns 0 if the thread already has the lock. This is + // the behavior choosen by the interface for this method. + return !isset( $this->namedLocksHeld[$lockName] ); } public function lock( $lockName, $method, $timeout = 5 ) { diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php index 7e6ddc3b8f..3085276c64 100644 --- a/includes/libs/rdbms/database/DatabaseMysqlBase.php +++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php @@ -1081,6 +1081,10 @@ abstract class DatabaseMysqlBase extends Database { * @since 1.20 */ public function lockIsFree( $lockName, $method ) { + if ( !parent::lockIsFree( $lockName, $method ) ) { + return false; // already held + } + $encName = $this->addQuotes( $this->makeLockName( $lockName ) ); $result = $this->query( "SELECT IS_FREE_LOCK($encName) AS lockstatus", $method ); $row = $this->fetchObject( $result ); @@ -1447,6 +1451,11 @@ abstract class DatabaseMysqlBase extends Database { return $index; } } + + protected function isTransactableQuery( $sql ) { + return parent::isTransactableQuery( $sql ) && + !preg_match( '/^SELECT\s+(GET|RELEASE|IS_FREE)_LOCK\(/', $sql ); + } } class_alias( DatabaseMysqlBase::class, 'DatabaseMysqlBase' ); diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index 7c07a07826..c6d5a1459f 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -202,6 +202,11 @@ class DatabasePostgres extends Database { return $this->conn ? pg_close( $this->conn ) : true; } + protected function isTransactableQuery( $sql ) { + return parent::isTransactableQuery( $sql ) && + !preg_match( '/^SELECT\s+pg_(try_|)advisory_\w+\(/', $sql ); + } + public function doQuery( $sql ) { $conn = $this->getBindingHandle(); @@ -1319,6 +1324,9 @@ SQL; } public function lockIsFree( $lockName, $method ) { + if ( !parent::lockIsFree( $lockName, $method ) ) { + return false; // already held + } // http://www.postgresql.org/docs/8.2/static/functions-admin.html#FUNCTIONS-ADVISORY-LOCKS $key = $this->addQuotes( $this->bigintFromLockName( $lockName ) ); $result = $this->query( "SELECT (CASE(pg_try_advisory_lock($key)) diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php index 0964dd55fe..d59bee38c1 100644 --- a/includes/libs/rdbms/database/IDatabase.php +++ b/includes/libs/rdbms/database/IDatabase.php @@ -1776,7 +1776,7 @@ interface IDatabase { public function setSchemaVars( $vars ); /** - * Check to see if a named lock is available (non-blocking) + * Check to see if a named lock is not locked by any thread (non-blocking) * * @param string $lockName Name of lock to poll * @param string $method Name of method calling us diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php index e131506729..b42a3672df 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php @@ -370,10 +370,34 @@ class DatabaseTest extends PHPUnit_Framework_TestCase { $this->assertFalse( (bool)$db->trxLevel(), "Transaction cleared." ); } + /** + * @covers Wikimedia\Rdbms\Database::getScopedLockAndFlush + * @covers Wikimedia\Rdbms\Database::lock + * @covers Wikimedia\Rdbms\Database::unlock + * @covers Wikimedia\Rdbms\Database::lockIsFree + */ public function testGetScopedLock() { $db = $this->getMockDB( [ 'isOpen' ] ); $db->method( 'isOpen' )->willReturn( true ); + $this->assertEquals( 0, $db->trxLevel() ); + $this->assertEquals( true, $db->lockIsFree( 'x', __METHOD__ ) ); + $this->assertEquals( true, $db->lock( 'x', __METHOD__ ) ); + $this->assertEquals( false, $db->lockIsFree( 'x', __METHOD__ ) ); + $this->assertEquals( true, $db->unlock( 'x', __METHOD__ ) ); + $this->assertEquals( true, $db->lockIsFree( 'x', __METHOD__ ) ); + $this->assertEquals( 0, $db->trxLevel() ); + + $db->setFlag( DBO_TRX ); + $this->assertEquals( true, $db->lockIsFree( 'x', __METHOD__ ) ); + $this->assertEquals( true, $db->lock( 'x', __METHOD__ ) ); + $this->assertEquals( false, $db->lockIsFree( 'x', __METHOD__ ) ); + $this->assertEquals( true, $db->unlock( 'x', __METHOD__ ) ); + $this->assertEquals( true, $db->lockIsFree( 'x', __METHOD__ ) ); + $db->clearFlag( DBO_TRX ); + + $this->assertEquals( 0, $db->trxLevel() ); + $db->setFlag( DBO_TRX ); try { $this->badLockingMethodImplicit( $db ); -- 2.20.1