rdbms: clean up DBO_TRX behavior for onTransaction* callbacks

* Make onTransactionIdle() wait until any transaction round
  is gone, even if there is no SQL transaction active. This
  is what onTransactionPreCommitOrIdle() already does.
* Decouple "transaction round mode" (DBO_TRX) from whether a
  round is active via a 'trxRoundId' LB info field. If rounds
  are enabled, but not is started, then the transaction state
  should be interpreted as "idle".
* Improve related documentation.
* Add more related unit tests.

Change-Id: I3ab18f577ec0375897fcb63f18f4ee2deeb436e9

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 523f7cd..e1f8759 100644 (file)
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -635,6 +635,20 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                );
        }
 
+       /**
+        * @return string|null
+        */
+       final protected function getTransactionRoundId() {
+               // If transaction round participation is enabled, see if one is active
+               if ( $this->getFlag( self::DBO_TRX ) ) {
+                       $id = $this->getLBInfo( 'trxRoundId' );
+
+                       return is_string( $id ) ? $id : null;
+               }
+
+               return null;
+       }
+
        public function pendingWriteQueryDuration( $type = self::ESTIMATE_TOTAL ) {
                if ( !$this->trxLevel ) {
                        return false;
@@ -3073,6 +3087,12 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
        }
 
        final public function onTransactionIdle( callable $callback, $fname = __METHOD__ ) {
+               if ( !$this->trxLevel && $this->getTransactionRoundId() ) {
+                       // Start an implicit transaction similar to how query() does
+                       $this->begin( __METHOD__, self::TRANSACTION_INTERNAL );
+                       $this->trxAutomatic = true;
+               }
+
                $this->trxIdleCallbacks[] = [ $callback, $fname ];
                if ( !$this->trxLevel ) {
                        $this->runOnTransactionIdleCallbacks( self::TRIGGER_IDLE );
@@ -3080,10 +3100,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
        }
 
        final public function onTransactionPreCommitOrIdle( callable $callback, $fname = __METHOD__ ) {
-               if ( $this->trxLevel || $this->getFlag( self::DBO_TRX ) ) {
-                       // As long as DBO_TRX is set, writes will accumulate until the load balancer issues
-                       // an implicit commit of all peer databases. This is true even if a transaction has
-                       // not yet been triggered by writes; make sure $callback runs *after* any such writes.
+               if ( !$this->trxLevel && $this->getTransactionRoundId() ) {
+                       // Start an implicit transaction similar to how query() does
+                       $this->begin( __METHOD__, self::TRANSACTION_INTERNAL );
+                       $this->trxAutomatic = true;
+               }
+
+               if ( $this->trxLevel ) {
                        $this->trxPreCommitCallbacks[] = [ $callback, $fname ];
                } else {
                        // No transaction is active nor will start implicitly, so make one for this callback

diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php
index 286d658..2be762c 100644 (file)
--- a/includes/libs/rdbms/database/DatabaseMysqlBase.php
+++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php
@@ -317,7 +317,7 @@ abstract class DatabaseMysqlBase extends Database {
        abstract protected function mysqlFetchObject( $res );
 
        /**
-        * @param ResultWrapper|resource $res
+        * @param IResultWrapper|resource $res
         * @return array|bool
         * @throws DBUnexpectedError
         */

diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php
index 07f1e23..e07b963 100644 (file)
--- a/includes/libs/rdbms/database/IDatabase.php
+++ b/includes/libs/rdbms/database/IDatabase.php
@@ -90,7 +90,7 @@ interface IDatabase {
        const DBO_NOBUFFER = 2;
        /** @var int Ignore query errors (internal use only!) */
        const DBO_IGNORE = 4;
-       /** @var int Autoatically start transaction on first query (work with ILoadBalancer rounds) */
+       /** @var int Automatically start a transaction before running a query if none is active */
        const DBO_TRX = 8;
        /** @var int Use DBO_TRX in non-CLI mode */
        const DBO_DEFAULT = 16;
@@ -253,7 +253,7 @@ interface IDatabase {
        public function writesPending();
 
        /**
-        * Returns true if there is a transaction open with possible write
+        * Returns true if there is a transaction/round open with possible write
         * queries or transaction pre-commit/idle callbacks waiting on it to finish.
         * This does *not* count recurring callbacks, e.g. from setTransactionListener().
         *
@@ -1490,13 +1490,19 @@ interface IDatabase {
        /**
         * Run a callback as soon as there is no transaction pending.
         * If there is a transaction and it is rolled back, then the callback is cancelled.
+        *
+        * When transaction round mode (DBO_TRX) is set, the callback will run at the end
+        * of the round, just after all peer transactions COMMIT. If the transaction round
+        * is rolled back, then the callback is cancelled.
+        *
         * Queries in the function will run in AUTO-COMMIT mode unless there are begin() calls.
         * Callbacks must commit any transactions that they begin.
         *
         * This is useful for updates to different systems or when separate transactions are needed.
         * For example, one might want to enqueue jobs into a system outside the database, but only
         * after the database is updated so that the jobs will see the data when they actually run.
-        * It can also be used for updates that easily cause deadlocks if locks are held too long.
+        * It can also be used for updates that easily suffer from lock timeouts and deadlocks,
+        * but where atomicity is not essential.
         *
         * Updates will execute in the order they were enqueued.
         *
@@ -1512,10 +1518,15 @@ interface IDatabase {
        /**
         * Run a callback before the current transaction commits or now if there is none.
         * If there is a transaction and it is rolled back, then the callback is cancelled.
+        *
+        * When transaction round mode (DBO_TRX) is set, the callback will run at the end
+        * of the round, just before all peer transactions COMMIT. If the transaction round
+        * is rolled back, then the callback is cancelled.
+        *
         * Callbacks must not start nor commit any transactions. If no transaction is active,
         * then a transaction will wrap the callback.
         *
-        * This is useful for updates that easily cause deadlocks if locks are held too long
+        * This is useful for updates that easily suffer from lock timeouts and deadlocks,
         * but where atomicity is strongly desired for these updates and some related updates.
         *
         * Updates will execute in the order they were enqueued.

diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
index 4de17b7..7c1b9d9 100644 (file)
--- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php
+++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
@@ -1423,6 +1423,12 @@ class LoadBalancer implements ILoadBalancer {
        }
 
        /**
+        * Make all DB servers with DBO_DEFAULT/DBO_TRX set join the transaction round
+        *
+        * Some servers may have neither flag enabled, meaning that they opt out of such
+        * transaction rounds and remain in auto-commit mode. Such behavior might be desired
+        * when a DB server is used for something like simple key/value storage.
+        *
         * @param IDatabase $conn
         */
        private function applyTransactionRoundFlags( IDatabase $conn ) {
@@ -1434,9 +1440,10 @@ class LoadBalancer implements ILoadBalancer {
                        // DBO_TRX is controlled entirely by CLI mode presence with DBO_DEFAULT.
                        // Force DBO_TRX even in CLI mode since a commit round is expected soon.
                        $conn->setFlag( $conn::DBO_TRX, $conn::REMEMBER_PRIOR );
-                       // If config has explicitly requested DBO_TRX be either on or off by not
-                       // setting DBO_DEFAULT, then respect that. Forcing no transactions is useful
-                       // for things like blob stores (ExternalStore) which want auto-commit mode.
+               }
+
+               if ( $conn->getFlag( $conn::DBO_TRX ) ) {
+                       $conn->setLBInfo( 'trxRoundId', $this->trxRoundId );
                }
        }
 
@@ -1448,6 +1455,10 @@ class LoadBalancer implements ILoadBalancer {
                        return; // transaction rounds do not apply to these connections
                }
 
+               if ( $conn->getFlag( $conn::DBO_TRX ) ) {
+                       $conn->setLBInfo( 'trxRoundId', false );
+               }
+
                if ( $conn->getFlag( $conn::DBO_DEFAULT ) ) {
                        $conn->restoreFlags( $conn::RESTORE_PRIOR );
                }

diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
index 24d848e..94b4e0d 100644 (file)
--- a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
+++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php
@@ -177,28 +177,27 @@ class DatabaseTest extends PHPUnit\Framework\TestCase {
        public function testTransactionIdle() {
                $db = $this->db;
 
-               $db->setFlag( DBO_TRX );
+               $db->clearFlag( DBO_TRX );
                $called = false;
                $flagSet = null;
-               $db->onTransactionIdle(
-                       function () use ( $db, &$flagSet, &$called ) {
-                               $called = true;
-                               $flagSet = $db->getFlag( DBO_TRX );
-                       },
-                       __METHOD__
-               );
-               $this->assertFalse( $flagSet, 'DBO_TRX off in callback' );
-               $this->assertTrue( $db->getFlag( DBO_TRX ), 'DBO_TRX restored to default' );
+               $callback = function () use ( $db, &$flagSet, &$called ) {
+                       $called = true;
+                       $flagSet = $db->getFlag( DBO_TRX );
+               };
+
+               $db->onTransactionIdle( $callback, __METHOD__ );
                $this->assertTrue( $called, 'Callback reached' );
+               $this->assertFalse( $flagSet, 'DBO_TRX off in callback' );
+               $this->assertFalse( $db->getFlag( DBO_TRX ), 'DBO_TRX still default' );
 
-               $db->clearFlag( DBO_TRX );
                $flagSet = null;
-               $db->onTransactionIdle(
-                       function () use ( $db, &$flagSet ) {
-                               $flagSet = $db->getFlag( DBO_TRX );
-                       },
-                       __METHOD__
-               );
+               $called = false;
+               $db->startAtomic( __METHOD__ );
+               $db->onTransactionIdle( $callback, __METHOD__ );
+               $this->assertFalse( $called, 'Callback not reached during TRX' );
+               $db->endAtomic( __METHOD__ );
+
+               $this->assertTrue( $called, 'Callback reached after COMMIT' );
                $this->assertFalse( $flagSet, 'DBO_TRX off in callback' );
                $this->assertFalse( $db->getFlag( DBO_TRX ), 'DBO_TRX restored to default' );
 
@@ -212,6 +211,56 @@ class DatabaseTest extends PHPUnit\Framework\TestCase {
                $this->assertFalse( $db->getFlag( DBO_TRX ), 'DBO_TRX restored to default' );
        }
 
+       /**
+        * @covers Wikimedia\Rdbms\Database::onTransactionIdle
+        * @covers Wikimedia\Rdbms\Database::runOnTransactionIdleCallbacks
+        */
+       public function testTransactionIdle_TRX() {
+               $db = $this->getMockDB( [ 'isOpen', 'ping' ] );
+               $db->method( 'isOpen' )->willReturn( true );
+               $db->method( 'ping' )->willReturn( true );
+               $db->setFlag( DBO_TRX );
+
+               $lbFactory = LBFactorySingle::newFromConnection( $db );
+               // Ask for the connection so that LB sets internal state
+               // about this connection being the master connection
+               $lb = $lbFactory->getMainLB();
+               $conn = $lb->openConnection( $lb->getWriterIndex() );
+               $this->assertSame( $db, $conn, 'Same DB instance' );
+               $this->assertTrue( $db->getFlag( DBO_TRX ), 'DBO_TRX is set' );
+
+               $called = false;
+               $flagSet = null;
+               $callback = function () use ( $db, &$flagSet, &$called ) {
+                       $called = true;
+                       $flagSet = $db->getFlag( DBO_TRX );
+               };
+
+               $db->onTransactionIdle( $callback, __METHOD__ );
+               $this->assertTrue( $called, 'Called when idle if DBO_TRX is set' );
+               $this->assertFalse( $flagSet, 'DBO_TRX off in callback' );
+               $this->assertTrue( $db->getFlag( DBO_TRX ), 'DBO_TRX still default' );
+
+               $called = false;
+               $lbFactory->beginMasterChanges( __METHOD__ );
+               $db->onTransactionIdle( $callback, __METHOD__ );
+               $this->assertFalse( $called, 'Not called when lb-transaction is active' );
+
+               $lbFactory->commitMasterChanges( __METHOD__ );
+               $this->assertTrue( $called, 'Called when lb-transaction is committed' );
+
+               $called = false;
+               $lbFactory->beginMasterChanges( __METHOD__ );
+               $db->onTransactionIdle( $callback, __METHOD__ );
+               $this->assertFalse( $called, 'Not called when lb-transaction is active' );
+
+               $lbFactory->rollbackMasterChanges( __METHOD__ );
+               $this->assertFalse( $called, 'Not called when lb-transaction is rolled back' );
+
+               $lbFactory->commitMasterChanges( __METHOD__ );
+               $this->assertFalse( $called, 'Not called in next round commit' );
+       }
+
        /**
         * @covers Wikimedia\Rdbms\Database::onTransactionPreCommitOrIdle
         * @covers Wikimedia\Rdbms\Database::runOnTransactionPreCommitCallbacks
@@ -250,12 +299,13 @@ class DatabaseTest extends PHPUnit\Framework\TestCase {
         * @covers Wikimedia\Rdbms\Database::runOnTransactionPreCommitCallbacks
         */
        public function testTransactionPreCommitOrIdle_TRX() {
-               $db = $this->getMockDB( [ 'isOpen' ] );
+               $db = $this->getMockDB( [ 'isOpen', 'ping' ] );
                $db->method( 'isOpen' )->willReturn( true );
+               $db->method( 'ping' )->willReturn( true );
                $db->setFlag( DBO_TRX );
 
                $lbFactory = LBFactorySingle::newFromConnection( $db );
-               // Ask for the connectin so that LB sets internal state
+               // Ask for the connection so that LB sets internal state
                // about this connection being the master connection
                $lb = $lbFactory->getMainLB();
                $conn = $lb->openConnection( $lb->getWriterIndex() );
@@ -267,11 +317,12 @@ class DatabaseTest extends PHPUnit\Framework\TestCase {
                        $called = true;
                };
                $db->onTransactionPreCommitOrIdle( $callback, __METHOD__ );
-               $this->assertFalse( $called, 'Not called when idle if DBO_TRX is set' );
+               $this->assertTrue( $called, 'Called when idle if DBO_TRX is set' );
 
+               $called = false;
                $lbFactory->beginMasterChanges( __METHOD__ );
+               $db->onTransactionPreCommitOrIdle( $callback, __METHOD__ );
                $this->assertFalse( $called, 'Not called when lb-transaction is active' );
-
                $lbFactory->commitMasterChanges( __METHOD__ );
                $this->assertTrue( $called, 'Called when lb-transaction is committed' );