From d4c31cf84115a7e447fdf1284c823000238981f9 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sun, 18 Mar 2018 22:26:11 -0700 Subject: [PATCH] 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 --- includes/libs/rdbms/database/Database.php | 31 ++++++- .../libs/rdbms/database/DatabaseMysqlBase.php | 2 +- includes/libs/rdbms/database/IDatabase.php | 19 +++- .../libs/rdbms/loadbalancer/LoadBalancer.php | 17 +++- .../libs/rdbms/database/DatabaseTest.php | 93 ++++++++++++++----- 5 files changed, 129 insertions(+), 33 deletions(-) diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 523f7cd462..e1f8759e53 100644 --- 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 286d65881e..2be762cca4 100644 --- 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 07f1e2336f..e07b963f3c 100644 --- 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 4de17b7547..7c1b9d98dd 100644 --- 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 24d848e406..94b4e0d995 100644 --- 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' ); -- 2.20.1