From f8c2486d1528fd00bbf4426b4721e66e255e7d2b Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Wed, 4 Apr 2018 16:29:18 -0700 Subject: [PATCH] rdbms: rename CONN_TRX_AUTO constant to CONN_TRX_AUTOCOMMIT The "AUTO" means AUTOCOMMIT, not "automatic transactions"/DBO_TRX, which is basically the opposite concept. The new name does not suffer from that ambiguity. Keep the old constant as an alias for backwards compatibility. Also remove LoadBalancer comment about non-existing field Change-Id: I63beeb061fc9be73f320308e4d6393b58628b8c8 --- includes/Storage/NameTableStore.php | 2 +- includes/jobqueue/JobQueueDB.php | 4 +-- .../libs/rdbms/loadbalancer/ILoadBalancer.php | 24 +++++++------ .../libs/rdbms/loadbalancer/LoadBalancer.php | 14 ++++---- includes/objectcache/SqlBagOStuff.php | 2 +- .../phpunit/includes/db/LoadBalancerTest.php | 36 ++++++++++--------- .../includes/jobqueue/JobQueueTest.php | 2 +- .../libs/rdbms/database/DBConnRefTest.php | 4 +-- 8 files changed, 47 insertions(+), 41 deletions(-) diff --git a/includes/Storage/NameTableStore.php b/includes/Storage/NameTableStore.php index a1eba74851..465f299770 100644 --- a/includes/Storage/NameTableStore.php +++ b/includes/Storage/NameTableStore.php @@ -138,7 +138,7 @@ class NameTableStore { // RACE: $name was already in the db, probably just inserted, so load from master // Use DBO_TRX to avoid missing inserts due to other threads or REPEATABLE-READs $table = $this->loadTable( - $this->getDBConnection( DB_MASTER, LoadBalancer::CONN_TRX_AUTO ) + $this->getDBConnection( DB_MASTER, LoadBalancer::CONN_TRX_AUTOCOMMIT ) ); $searchResult = array_search( $name, $table, true ); if ( $searchResult === false ) { diff --git a/includes/jobqueue/JobQueueDB.php b/includes/jobqueue/JobQueueDB.php index b68fdaefb3..f01ba63f02 100644 --- a/includes/jobqueue/JobQueueDB.php +++ b/includes/jobqueue/JobQueueDB.php @@ -190,7 +190,7 @@ class JobQueueDB extends JobQueue { // If the connection is busy with a transaction, then defer the job writes // until right before the main round commit step. Any errors that bubble // up will rollback the main commit round. - // b) mysql/postgres; DB connection is generally a separate CONN_TRX_AUTO handle. + // b) mysql/postgres; DB connection is generally a separate CONN_TRX_AUTOCOMMIT handle. // No transaction is active nor will be started by writes, so enqueue the jobs // now so that any errors will show up immediately as the interface expects. Any // errors that bubble up will rollback the main commit round. @@ -780,7 +780,7 @@ class JobQueueDB extends JobQueue { return ( $lb->getServerType( $lb->getWriterIndex() ) !== 'sqlite' ) // Keep a separate connection to avoid contention and deadlocks; // However, SQLite has the opposite behavior due to DB-level locking. - ? $lb->getConnectionRef( $index, [], $this->wiki, $lb::CONN_TRX_AUTO ) + ? $lb->getConnectionRef( $index, [], $this->wiki, $lb::CONN_TRX_AUTOCOMMIT ) // Jobs insertion will be defered until the PRESEND stage to reduce contention. : $lb->getConnectionRef( $index, [], $this->wiki ); } diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php index 767cc49f59..3c07d0f9f1 100644 --- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php @@ -85,6 +85,8 @@ interface ILoadBalancer { const DOMAIN_ANY = ''; /** @var int DB handle should have DBO_TRX disabled and the caller will leave it as such */ + const CONN_TRX_AUTOCOMMIT = 1; + /** @var int Alias for CONN_TRX_AUTOCOMMIT for b/c; deprecated since 1.31 */ const CONN_TRX_AUTO = 1; /** @@ -172,11 +174,11 @@ interface ILoadBalancer { /** * Get a connection handle by server index * - * The CONN_TRX_AUTO flag is ignored for databases with ATTR_DB_LEVEL_LOCKING + * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes() * can be used to check such flags beforehand. * - * If the caller uses $domain or sets CONN_TRX_AUTO in $flags, then it must also + * If the caller uses $domain or sets CONN_TRX_AUTOCOMMIT in $flags, then it must also * call ILoadBalancer::reuseConnection() on the handle when finished using it. * In all other cases, this is not necessary, though not harmful either. * @@ -208,7 +210,7 @@ interface ILoadBalancer { * * The handle's methods simply wrap those of a Database handle * - * The CONN_TRX_AUTO flag is ignored for databases with ATTR_DB_LEVEL_LOCKING + * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes() * can be used to check such flags beforehand. * @@ -217,7 +219,7 @@ interface ILoadBalancer { * @param int $i Server index or DB_MASTER/DB_REPLICA * @param array|string|bool $groups Query group(s), or false for the generic reader * @param string|bool $domain Domain ID, or false for the current domain - * @param int $flags Bitfield of CONN_* class constants (e.g. CONN_TRX_AUTO) + * @param int $flags Bitfield of CONN_* class constants (e.g. CONN_TRX_AUTOCOMMIT) * @return DBConnRef */ public function getConnectionRef( $i, $groups = [], $domain = false, $flags = 0 ); @@ -227,7 +229,7 @@ interface ILoadBalancer { * * The handle's methods simply wrap those of a Database handle * - * The CONN_TRX_AUTO flag is ignored for databases with ATTR_DB_LEVEL_LOCKING + * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes() * can be used to check such flags beforehand. * @@ -236,7 +238,7 @@ interface ILoadBalancer { * @param int $i Server index or DB_MASTER/DB_REPLICA * @param array|string|bool $groups Query group(s), or false for the generic reader * @param string|bool $domain Domain ID, or false for the current domain - * @param int $flags Bitfield of CONN_* class constants (e.g. CONN_TRX_AUTO) + * @param int $flags Bitfield of CONN_* class constants (e.g. CONN_TRX_AUTOCOMMIT) * @return DBConnRef */ public function getLazyConnectionRef( $i, $groups = [], $domain = false, $flags = 0 ); @@ -246,7 +248,7 @@ interface ILoadBalancer { * * The handle's methods simply wrap those of a Database handle * - * The CONN_TRX_AUTO flag is ignored for databases with ATTR_DB_LEVEL_LOCKING + * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes() * can be used to check such flags beforehand. * @@ -255,7 +257,7 @@ interface ILoadBalancer { * @param int $db Server index or DB_MASTER/DB_REPLICA * @param array|string|bool $groups Query group(s), or false for the generic reader * @param string|bool $domain Domain ID, or false for the current domain - * @param int $flags Bitfield of CONN_* class constants (e.g. CONN_TRX_AUTO) + * @param int $flags Bitfield of CONN_* class constants (e.g. CONN_TRX_AUTOCOMMIT) * @return MaintainableDBConnRef */ public function getMaintenanceConnectionRef( $db, $groups = [], $domain = false, $flags = 0 ); @@ -266,11 +268,11 @@ interface ILoadBalancer { * The index must be an actual index into the array. If a connection to the server is * already open and not considered an "in use" foreign connection, this simply returns it. * - * Avoid using CONN_TRX_AUTO for databases with ATTR_DB_LEVEL_LOCKING (e.g. sqlite) in + * Avoid using CONN_TRX_AUTOCOMMIT for databases with ATTR_DB_LEVEL_LOCKING (e.g. sqlite) in * order to avoid deadlocks. ILoadBalancer::getServerAttributes() can be used to check * such flags beforehand. * - * If the caller uses $domain or sets CONN_TRX_AUTO in $flags, then it must also + * If the caller uses $domain or sets CONN_TRX_AUTOCOMMIT in $flags, then it must also * call ILoadBalancer::reuseConnection() on the handle when finished using it. * In all other cases, this is not necessary, though not harmful either. * @@ -278,7 +280,7 @@ interface ILoadBalancer { * * @param int $i Server index (does not support DB_MASTER/DB_REPLICA) * @param string|bool $domain Domain ID, or false for the current domain - * @param int $flags Bitfield of CONN_* class constants (e.g. CONN_TRX_AUTO) + * @param int $flags Bitfield of CONN_* class constants (e.g. CONN_TRX_AUTOCOMMIT) * @return Database|bool Returns false on errors * @throws DBAccessError */ diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index 7c1b9d98dd..e28e222b75 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -568,7 +568,6 @@ class LoadBalancer implements ILoadBalancer { if ( !empty( $connsByServer[$i] ) ) { /** @var IDatabase[] $serverConns */ $serverConns = $connsByServer[$i]; - return reset( $serverConns ); } } @@ -682,7 +681,7 @@ class LoadBalancer implements ILoadBalancer { $domain = false; // local connection requested } - if ( ( $flags & self::CONN_TRX_AUTO ) === self::CONN_TRX_AUTO ) { + if ( ( $flags & self::CONN_TRX_AUTOCOMMIT ) === self::CONN_TRX_AUTOCOMMIT ) { // Assuming all servers are of the same type (or similar), which is overwhelmingly // the case, use the master server information to get the attributes. The information // for $i cannot be used since it might be DB_REPLICA, which might require connection @@ -693,8 +692,9 @@ class LoadBalancer implements ILoadBalancer { // rows (e.g. FOR UPDATE) or (b) make small commits during a larger transactions // to reduce lock contention. None of these apply for sqlite and using separate // connections just causes self-deadlocks. - $flags &= ~self::CONN_TRX_AUTO; - $this->connLogger->info( __METHOD__ . ': ignoring CONN_TRX_AUTO to avoid deadlocks.' ); + $flags &= ~self::CONN_TRX_AUTOCOMMIT; + $this->connLogger->info( __METHOD__ . + ': ignoring CONN_TRX_AUTOCOMMIT to avoid deadlocks.' ); } } @@ -852,7 +852,7 @@ class LoadBalancer implements ILoadBalancer { // main set of DB connections but rather its own pool since: // a) those are usually set to implicitly use transaction rounds via DBO_TRX // b) those must support the use of explicit transaction rounds via beginMasterChanges() - $autoCommit = ( ( $flags & self::CONN_TRX_AUTO ) == self::CONN_TRX_AUTO ); + $autoCommit = ( ( $flags & self::CONN_TRX_AUTOCOMMIT ) == self::CONN_TRX_AUTOCOMMIT ); if ( $domain !== false ) { // Connection is to a foreign domain @@ -930,7 +930,7 @@ class LoadBalancer implements ILoadBalancer { $domainInstance = DatabaseDomain::newFromId( $domain ); $dbName = $domainInstance->getDatabase(); $prefix = $domainInstance->getTablePrefix(); - $autoCommit = ( ( $flags & self::CONN_TRX_AUTO ) == self::CONN_TRX_AUTO ); + $autoCommit = ( ( $flags & self::CONN_TRX_AUTOCOMMIT ) == self::CONN_TRX_AUTOCOMMIT ); if ( $autoCommit ) { $connFreeKey = self::KEY_FOREIGN_FREE_NOROUND; @@ -1212,7 +1212,7 @@ class LoadBalancer implements ILoadBalancer { } public function closeConnection( IDatabase $conn ) { - $serverIndex = $conn->getLBInfo( 'serverIndex' ); // second index level of mConns + $serverIndex = $conn->getLBInfo( 'serverIndex' ); foreach ( $this->conns as $type => $connsByServer ) { if ( !isset( $connsByServer[$serverIndex] ) ) { continue; diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php index 6d35658151..8ff14ed788 100644 --- a/includes/objectcache/SqlBagOStuff.php +++ b/includes/objectcache/SqlBagOStuff.php @@ -181,7 +181,7 @@ class SqlBagOStuff extends BagOStuff { $index = $this->replicaOnly ? DB_REPLICA : DB_MASTER; if ( $lb->getServerType( $lb->getWriterIndex() ) !== 'sqlite' ) { // Keep a separate connection to avoid contention and deadlocks - $db = $lb->getConnection( $index, [], false, $lb::CONN_TRX_AUTO ); + $db = $lb->getConnection( $index, [], false, $lb::CONN_TRX_AUTOCOMMIT ); // @TODO: Use a blank trx profiler to ignore expections as this is a cache } else { // However, SQLite has the opposite behavior due to DB-level locking. diff --git a/tests/phpunit/includes/db/LoadBalancerTest.php b/tests/phpunit/includes/db/LoadBalancerTest.php index 6ced540ffd..cd48d90fa2 100644 --- a/tests/phpunit/includes/db/LoadBalancerTest.php +++ b/tests/phpunit/includes/db/LoadBalancerTest.php @@ -67,18 +67,20 @@ class LoadBalancerTest extends MediaWikiTestCase { $this->assertTrue( $dbr->getLBInfo( 'master' ), 'DB_REPLICA also gets the master' ); $this->assertTrue( $dbr->getFlag( $dbw::DBO_TRX ), "DBO_TRX set on replica" ); - $dbwAuto = $lb->getConnection( DB_MASTER, [], false, $lb::CONN_TRX_AUTO ); - $this->assertFalse( $dbwAuto->getFlag( $dbw::DBO_TRX ), "No DBO_TRX with CONN_TRX_AUTO" ); + $dbwAuto = $lb->getConnection( DB_MASTER, [], false, $lb::CONN_TRX_AUTOCOMMIT ); + $this->assertFalse( + $dbwAuto->getFlag( $dbw::DBO_TRX ), "No DBO_TRX with CONN_TRX_AUTOCOMMIT" ); $this->assertTrue( $dbw->getFlag( $dbw::DBO_TRX ), "DBO_TRX still set on master" ); - $this->assertNotEquals( $dbw, $dbwAuto, "CONN_TRX_AUTO uses separate connection" ); + $this->assertNotEquals( $dbw, $dbwAuto, "CONN_TRX_AUTOCOMMIT uses separate connection" ); - $dbrAuto = $lb->getConnection( DB_REPLICA, [], false, $lb::CONN_TRX_AUTO ); - $this->assertFalse( $dbrAuto->getFlag( $dbw::DBO_TRX ), "No DBO_TRX with CONN_TRX_AUTO" ); + $dbrAuto = $lb->getConnection( DB_REPLICA, [], false, $lb::CONN_TRX_AUTOCOMMIT ); + $this->assertFalse( + $dbrAuto->getFlag( $dbw::DBO_TRX ), "No DBO_TRX with CONN_TRX_AUTOCOMMIT" ); $this->assertTrue( $dbr->getFlag( $dbw::DBO_TRX ), "DBO_TRX still set on replica" ); - $this->assertNotEquals( $dbr, $dbrAuto, "CONN_TRX_AUTO uses separate connection" ); + $this->assertNotEquals( $dbr, $dbrAuto, "CONN_TRX_AUTOCOMMIT uses separate connection" ); - $dbwAuto2 = $lb->getConnection( DB_MASTER, [], false, $lb::CONN_TRX_AUTO ); - $this->assertEquals( $dbwAuto2, $dbwAuto, "CONN_TRX_AUTO reuses connections" ); + $dbwAuto2 = $lb->getConnection( DB_MASTER, [], false, $lb::CONN_TRX_AUTOCOMMIT ); + $this->assertEquals( $dbwAuto2, $dbwAuto, "CONN_TRX_AUTOCOMMIT reuses connections" ); $lb->closeAll(); } @@ -135,18 +137,20 @@ class LoadBalancerTest extends MediaWikiTestCase { $this->assertTrue( $dbr->getFlag( $dbw::DBO_TRX ), "DBO_TRX set on replica" ); $this->assertWriteForbidden( $dbr ); - $dbwAuto = $lb->getConnection( DB_MASTER, [], false, $lb::CONN_TRX_AUTO ); - $this->assertFalse( $dbwAuto->getFlag( $dbw::DBO_TRX ), "No DBO_TRX with CONN_TRX_AUTO" ); + $dbwAuto = $lb->getConnection( DB_MASTER, [], false, $lb::CONN_TRX_AUTOCOMMIT ); + $this->assertFalse( + $dbwAuto->getFlag( $dbw::DBO_TRX ), "No DBO_TRX with CONN_TRX_AUTOCOMMIT" ); $this->assertTrue( $dbw->getFlag( $dbw::DBO_TRX ), "DBO_TRX still set on master" ); - $this->assertNotEquals( $dbw, $dbwAuto, "CONN_TRX_AUTO uses separate connection" ); + $this->assertNotEquals( $dbw, $dbwAuto, "CONN_TRX_AUTOCOMMIT uses separate connection" ); - $dbrAuto = $lb->getConnection( DB_REPLICA, [], false, $lb::CONN_TRX_AUTO ); - $this->assertFalse( $dbrAuto->getFlag( $dbw::DBO_TRX ), "No DBO_TRX with CONN_TRX_AUTO" ); + $dbrAuto = $lb->getConnection( DB_REPLICA, [], false, $lb::CONN_TRX_AUTOCOMMIT ); + $this->assertFalse( + $dbrAuto->getFlag( $dbw::DBO_TRX ), "No DBO_TRX with CONN_TRX_AUTOCOMMIT" ); $this->assertTrue( $dbr->getFlag( $dbw::DBO_TRX ), "DBO_TRX still set on replica" ); - $this->assertNotEquals( $dbr, $dbrAuto, "CONN_TRX_AUTO uses separate connection" ); + $this->assertNotEquals( $dbr, $dbrAuto, "CONN_TRX_AUTOCOMMIT uses separate connection" ); - $dbwAuto2 = $lb->getConnection( DB_MASTER, [], false, $lb::CONN_TRX_AUTO ); - $this->assertEquals( $dbwAuto2, $dbwAuto, "CONN_TRX_AUTO reuses connections" ); + $dbwAuto2 = $lb->getConnection( DB_MASTER, [], false, $lb::CONN_TRX_AUTOCOMMIT ); + $this->assertEquals( $dbwAuto2, $dbwAuto, "CONN_TRX_AUTOCOMMIT reuses connections" ); $lb->closeAll(); } diff --git a/tests/phpunit/includes/jobqueue/JobQueueTest.php b/tests/phpunit/includes/jobqueue/JobQueueTest.php index 0625edd8b5..64dde77818 100644 --- a/tests/phpunit/includes/jobqueue/JobQueueTest.php +++ b/tests/phpunit/includes/jobqueue/JobQueueTest.php @@ -387,7 +387,7 @@ class JobQueueTest extends MediaWikiTestCase { class JobQueueDBSingle extends JobQueueDB { protected function getDB( $index ) { $lb = MediaWikiServices::getInstance()->getDBLoadBalancer(); - // Override to not use CONN_TRX_AUTO so that we see the same temporary `job` table + // Override to not use CONN_TRX_AUTOCOMMIT so that we see the same temporary `job` table return $lb->getConnection( $index, [], $this->wiki ); } } diff --git a/tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php b/tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php index d1f961a809..b370508811 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php @@ -67,7 +67,7 @@ class DBConnRefTest extends PHPUnit\Framework\TestCase { $lb->expects( $this->once() ) ->method( 'getConnection' ) - ->with( DB_MASTER, [ 'test' ], 'dummy', ILoadBalancer::CONN_TRX_AUTO ) + ->with( DB_MASTER, [ 'test' ], 'dummy', ILoadBalancer::CONN_TRX_AUTOCOMMIT ) ->willReturnCallback( function () { return $this->getDatabaseMock(); @@ -76,7 +76,7 @@ class DBConnRefTest extends PHPUnit\Framework\TestCase { $ref = new DBConnRef( $lb, - [ DB_MASTER, [ 'test' ], 'dummy', ILoadBalancer::CONN_TRX_AUTO ] + [ DB_MASTER, [ 'test' ], 'dummy', ILoadBalancer::CONN_TRX_AUTOCOMMIT ] ); $this->assertInstanceOf( ResultWrapper::class, $ref->select( 'whatever', '*' ) ); -- 2.20.1