Merge "rdbms: rename CONN_TRX_AUTO constant to CONN_TRX_AUTOCOMMIT"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Thu, 12 Apr 2018 20:38:58 +0000 (20:38 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Thu, 12 Apr 2018 20:38:58 +0000 (20:38 +0000)
includes/Storage/NameTableStore.php
includes/jobqueue/JobQueueDB.php
includes/libs/rdbms/loadbalancer/ILoadBalancer.php
includes/libs/rdbms/loadbalancer/LoadBalancer.php
includes/objectcache/SqlBagOStuff.php
tests/phpunit/includes/db/LoadBalancerTest.php
tests/phpunit/includes/jobqueue/JobQueueTest.php
tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php

index a1eba74..465f299 100644 (file)
@@ -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 ) {
index b68fdae..f01ba63 100644 (file)
@@ -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 );
        }
index 715f4e4..ae4362d 100644 (file)
@@ -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;
 
        /**
@@ -173,11 +175,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.
         *
@@ -209,7 +211,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.
         *
@@ -218,7 +220,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 );
@@ -228,7 +230,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.
         *
@@ -237,7 +239,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 );
@@ -247,7 +249,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.
         *
@@ -256,7 +258,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 );
@@ -267,11 +269,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.
         *
@@ -279,7 +281,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
         */
index 94acc1e..c587b42 100644 (file)
@@ -575,7 +575,6 @@ class LoadBalancer implements ILoadBalancer {
                        if ( !empty( $connsByServer[$i] ) ) {
                                /** @var IDatabase[] $serverConns */
                                $serverConns = $connsByServer[$i];
-
                                return reset( $serverConns );
                        }
                }
@@ -689,7 +688,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
@@ -700,8 +699,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.' );
                        }
                }
 
@@ -859,7 +859,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
@@ -937,7 +937,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;
@@ -1220,7 +1220,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;
index 6d35658..8ff14ed 100644 (file)
@@ -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.
index 6ced540..cd48d90 100644 (file)
@@ -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();
        }
index 0625edd..64dde77 100644 (file)
@@ -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 );
        }
 }
index bc9d9ea..c3cddc6 100644 (file)
@@ -69,7 +69,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();
@@ -78,7 +78,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', '*' ) );