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

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index c6c7a9b..9a8996c 100644 (file)
--- 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 ) {
        }
 
        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 ) {
        }
 
        public function lock( $lockName, $method, $timeout = 5 ) {

diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php
index 7e6ddc3..3085276 100644 (file)
--- 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 ) {
         * @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 );
                $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;
                }
        }
                        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' );
 }
 
 class_alias( DatabaseMysqlBase::class, 'DatabaseMysqlBase' );

diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php
index 7c07a07..c6d5a14 100644 (file)
--- 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;
        }
 
                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();
 
        public function doQuery( $sql ) {
                $conn = $this->getBindingHandle();
 


@@ -1319,6 +1324,9 @@ SQL;
        }
 
        public function lockIsFree( $lockName, $method ) {
        }
 
        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))
                // 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 0964dd5..d59bee3 100644 (file)
--- a/includes/libs/rdbms/database/IDatabase.php
+++ b/includes/libs/rdbms/database/IDatabase.php
@@ -1776,7 +1776,7 @@ interface IDatabase {
        public function setSchemaVars( $vars );
 
        /**
        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
         *
         * @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 e131506..b42a367 100644 (file)
--- 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." );
        }
 
                $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 );
 
        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 );
                $db->setFlag( DBO_TRX );
                try {
                        $this->badLockingMethodImplicit( $db );