rdbms: rename CONN_TRX_AUTO constant to CONN_TRX_AUTOCOMMIT
authorAaron Schulz <aschulz@wikimedia.org>
Wed, 4 Apr 2018 23:29:18 +0000 (16:29 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Thu, 12 Apr 2018 20:01:52 +0000 (13:01 -0700)
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
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 767cc49..3c07d0f 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;
 
        /**
@@ -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
         */
index 7c1b9d9..e28e222 100644 (file)
@@ -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;
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 d1f961a..b370508 100644 (file)
@@ -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', '*' ) );