rdbms: Support secondary autocommit connections in LoadBalancer
authorAaron Schulz <aschulz@wikimedia.org>
Thu, 20 Jul 2017 03:51:54 +0000 (20:51 -0700)
committerKrinkle <krinklemail@gmail.com>
Fri, 18 Aug 2017 01:28:34 +0000 (01:28 +0000)
This is useful for things like SqlBagOStuff and JobQueueDB
being able to write in auto-commit mode while the main
transaction round still goes on.

SqlBagOStuff already had this sort of logic.

JobQueueDB now also takes this approach so it does not have to
defer insertion except for sqlite. This makes callers more likely
to know when insertion failed or not.

Make sure LoadBalancer sets "master"/"replica" LB info itself;
this fixes a bug found in the new tests.

Bug: T140338
Bug: T42451
Change-Id: I4199a9598d7afbf976a6efa8ed84b85b56da02bd

includes/jobqueue/JobQueueDB.php
includes/libs/rdbms/database/DBConnRef.php
includes/libs/rdbms/loadbalancer/ILoadBalancer.php
includes/libs/rdbms/loadbalancer/LoadBalancer.php
includes/objectcache/SqlBagOStuff.php
tests/phpunit/includes/db/LoadBalancerTest.php [new file with mode: 0644]

index b7cc133..b5f331b 100644 (file)
@@ -184,15 +184,22 @@ class JobQueueDB extends JobQueue {
         * @return void
         */
        protected function doBatchPush( array $jobs, $flags ) {
-               DeferredUpdates::addUpdate(
-                       new AutoCommitUpdate(
-                               $this->getMasterDB(),
-                               __METHOD__,
-                               function ( IDatabase $dbw, $fname ) use ( $jobs, $flags ) {
-                                       $this->doBatchPushInternal( $dbw, $jobs, $flags, $fname );
-                               }
-                       ),
-                       DeferredUpdates::PRESEND
+               $dbw = $this->getMasterDB();
+               // In general, there will be two cases here:
+               // a) sqlite; DB connection is probably a regular round-aware handle.
+               // 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.
+               // 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.
+               $fname = __METHOD__;
+               $dbw->onTransactionPreCommitOrIdle(
+                       function () use ( $dbw, $jobs, $flags, $fname ) {
+                               $this->doBatchPushInternal( $dbw, $jobs, $flags, $fname );
+                       },
+                       $fname
                );
        }
 
@@ -771,7 +778,12 @@ class JobQueueDB extends JobQueue {
                        ? $lbFactory->getExternalLB( $this->cluster )
                        : $lbFactory->getMainLB( $this->wiki );
 
-               return $lb->getConnectionRef( $index, [], $this->wiki );
+               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 )
+                       // Jobs insertion will be defered until the PRESEND stage to reduce contention.
+                       : $lb->getConnectionRef( $index, [], $this->wiki );
        }
 
        /**
index eb0e954..ef2953e 100644 (file)
@@ -23,16 +23,17 @@ class DBConnRef implements IDatabase {
        const FLD_INDEX = 0;
        const FLD_GROUP = 1;
        const FLD_DOMAIN = 2;
+       const FLD_FLAGS = 3;
 
        /**
         * @param ILoadBalancer $lb Connection manager for $conn
-        * @param Database|array $conn New connection handle or (server index, query groups, domain)
+        * @param Database|array $conn Database handle or (server index, query groups, domain, flags)
         */
        public function __construct( ILoadBalancer $lb, $conn ) {
                $this->lb = $lb;
                if ( $conn instanceof Database ) {
                        $this->conn = $conn; // live handle
-               } elseif ( count( $conn ) >= 3 && $conn[self::FLD_DOMAIN] !== false ) {
+               } elseif ( count( $conn ) >= 4 && $conn[self::FLD_DOMAIN] !== false ) {
                        $this->params = $conn;
                } else {
                        throw new InvalidArgumentException( "Missing lazy connection arguments." );
@@ -41,8 +42,8 @@ class DBConnRef implements IDatabase {
 
        function __call( $name, array $arguments ) {
                if ( $this->conn === null ) {
-                       list( $db, $groups, $wiki ) = $this->params;
-                       $this->conn = $this->lb->getConnection( $db, $groups, $wiki );
+                       list( $db, $groups, $wiki, $flags ) = $this->params;
+                       $this->conn = $this->lb->getConnection( $db, $groups, $wiki, $flags );
                }
 
                return call_user_func_array( [ $this->conn, $name ], $arguments );
index c940392..22a5805 100644 (file)
@@ -76,14 +76,17 @@ use InvalidArgumentException;
  * @ingroup Database
  */
 interface ILoadBalancer {
-       /** @var integer Request a replica DB connection */
+       /** @var int Request a replica DB connection */
        const DB_REPLICA = -1;
-       /** @var integer Request a master DB connection */
+       /** @var int Request a master DB connection */
        const DB_MASTER = -2;
 
        /** @var string Domain specifier when no specific database needs to be selected */
        const DOMAIN_ANY = '';
 
+       /** @var int DB handle should have DBO_TRX disabled and the caller will leave it as such */
+       const CONN_TRX_AUTO = 1;
+
        /**
         * Construct a manager of IDatabase connection objects
         *
@@ -168,14 +171,17 @@ interface ILoadBalancer {
        /**
         * Get a connection by index
         *
+        * Avoid using CONN_TRX_AUTO with sqlite (e.g. check getServerType() first)
+        *
         * @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
         *
         * @throws DBError
         * @return Database
         */
-       public function getConnection( $i, $groups = [], $domain = false );
+       public function getConnection( $i, $groups = [], $domain = false, $flags = 0 );
 
        /**
         * Mark a foreign connection as being available for reuse under a different DB domain
@@ -193,42 +199,51 @@ interface ILoadBalancer {
         *
         * The handle's methods simply wrap those of a Database handle
         *
+        * Avoid using CONN_TRX_AUTO with sqlite (e.g. check getServerType() first)
+        *
         * @see ILoadBalancer::getConnection() for parameter information
         *
         * @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)
         * @return DBConnRef
         */
-       public function getConnectionRef( $i, $groups = [], $domain = false );
+       public function getConnectionRef( $i, $groups = [], $domain = false, $flags = 0 );
 
        /**
         * Get a database connection handle reference without connecting yet
         *
         * The handle's methods simply wrap those of a Database handle
         *
+        * Avoid using CONN_TRX_AUTO with sqlite (e.g. check getServerType() first)
+        *
         * @see ILoadBalancer::getConnection() for parameter information
         *
         * @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)
         * @return DBConnRef
         */
-       public function getLazyConnectionRef( $i, $groups = [], $domain = false );
+       public function getLazyConnectionRef( $i, $groups = [], $domain = false, $flags = 0 );
 
        /**
         * Get a maintenance database connection handle reference for migrations and schema changes
         *
         * The handle's methods simply wrap those of a Database handle
         *
+        * Avoid using CONN_TRX_AUTO with sqlite (e.g. check getServerType() first)
+        *
         * @see ILoadBalancer::getConnection() for parameter information
         *
         * @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)
         * @return MaintainableDBConnRef
         */
-       public function getMaintenanceConnectionRef( $db, $groups = [], $domain = false );
+       public function getMaintenanceConnectionRef( $db, $groups = [], $domain = false, $flags = 0 );
 
        /**
         * Open a connection to the server given by the specified index
@@ -236,14 +251,17 @@ 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 with sqlite (e.g. check getServerType() first)
+        *
         * @note If disable() was called on this LoadBalancer, this method will throw a DBAccessError.
         *
         * @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)
         * @return Database|bool Returns false on errors
         * @throws DBAccessError
         */
-       public function openConnection( $i, $domain = false );
+       public function openConnection( $i, $domain = false, $flags = 0 );
 
        /**
         * @return int
@@ -253,7 +271,7 @@ interface ILoadBalancer {
        /**
         * Returns true if the specified index is a valid server index
         *
-        * @param string $i
+        * @param int $i
         * @return bool
         */
        public function haveIndex( $i );
@@ -261,7 +279,7 @@ interface ILoadBalancer {
        /**
         * Returns true if the specified index is valid and has non-zero load
         *
-        * @param string $i
+        * @param int $i
         * @return bool
         */
        public function isNonZeroLoad( $i );
@@ -275,12 +293,21 @@ interface ILoadBalancer {
 
        /**
         * Get the host name or IP address of the server with the specified index
-        * Prefer a readable name if available.
-        * @param string $i
-        * @return string
+        *
+        * @param int $i
+        * @return string Readable name if available or IP/host otherwise
         */
        public function getServerName( $i );
 
+       /**
+        * Get DB type of the server with the specified index
+        *
+        * @param int $i
+        * @return string One of (mysql,postgres,sqlite,...) or "unknown" for bad indexes
+        * @since 1.30
+        */
+       public function getServerType( $i );
+
        /**
         * Return the server info structure for a given index, or false if the index is invalid.
         * @param int $i
index 1df2409..f64cd56 100644 (file)
@@ -41,7 +41,7 @@ use Exception;
 class LoadBalancer implements ILoadBalancer {
        /** @var array[] Map of (server index => server config array) */
        private $mServers;
-       /** @var Database[][][] Map of local/foreignUsed/foreignFree => server index => IDatabase[] */
+       /** @var Database[][][] Map of (connection category => server index => IDatabase[]) */
        private $mConns;
        /** @var float[] Map of (server index => weight) */
        private $mLoads;
@@ -128,11 +128,22 @@ class LoadBalancer implements ILoadBalancer {
        const KEY_FOREIGN_FREE = 'foreignFree';
        const KEY_FOREIGN_INUSE = 'foreignInUse';
 
+       const KEY_LOCAL_NOROUND = 'localAutoCommit';
+       const KEY_FOREIGN_FREE_NOROUND = 'foreignFreeAutoCommit';
+       const KEY_FOREIGN_INUSE_NOROUND = 'foreignInUseAutoCommit';
+
        public function __construct( array $params ) {
                if ( !isset( $params['servers'] ) ) {
                        throw new InvalidArgumentException( __CLASS__ . ': missing servers parameter' );
                }
                $this->mServers = $params['servers'];
+               foreach ( $this->mServers as $i => $server ) {
+                       if ( $i == 0 ) {
+                               $this->mServers[$i]['master'] = true;
+                       } else {
+                               $this->mServers[$i]['replica'] = true;
+                       }
+               }
 
                $this->localDomain = isset( $params['localDomain'] )
                        ? DatabaseDomain::newFromId( $params['localDomain'] )
@@ -150,9 +161,14 @@ class LoadBalancer implements ILoadBalancer {
 
                $this->mReadIndex = -1;
                $this->mConns = [
+                       // Connection were transaction rounds may be applied
                        self::KEY_LOCAL => [],
                        self::KEY_FOREIGN_INUSE => [],
-                       self::KEY_FOREIGN_FREE => []
+                       self::KEY_FOREIGN_FREE => [],
+                       // Auto-committing counterpart connections that ignore transaction rounds
+                       self::KEY_LOCAL_NOROUND => [],
+                       self::KEY_FOREIGN_INUSE_NOROUND => [],
+                       self::KEY_FOREIGN_FREE_NOROUND => []
                ];
                $this->mLoads = [];
                $this->mWaitForPos = false;
@@ -601,16 +617,7 @@ class LoadBalancer implements ILoadBalancer {
                return $ok;
        }
 
-       /**
-        * @see ILoadBalancer::getConnection()
-        *
-        * @param int $i
-        * @param array $groups
-        * @param bool $domain
-        * @return Database
-        * @throws DBConnectionError
-        */
-       public function getConnection( $i, $groups = [], $domain = false ) {
+       public function getConnection( $i, $groups = [], $domain = false, $flags = 0 ) {
                if ( $i === null || $i === false ) {
                        throw new InvalidArgumentException( 'Attempt to call ' . __METHOD__ .
                                ' with invalid server index' );
@@ -657,7 +664,7 @@ class LoadBalancer implements ILoadBalancer {
                }
 
                # Now we have an explicit index into the servers array
-               $conn = $this->openConnection( $i, $domain );
+               $conn = $this->openConnection( $i, $domain, $flags );
                if ( !$conn ) {
                        // Throw an exception
                        $this->reportConnectionError();
@@ -707,20 +714,29 @@ class LoadBalancer implements ILoadBalancer {
                        return; // DBConnRef handle probably survived longer than the LoadBalancer
                }
 
+               if ( $conn->getLBInfo( 'autoCommitOnly' ) ) {
+                       $connFreeKey = self::KEY_FOREIGN_FREE_NOROUND;
+                       $connInUseKey = self::KEY_FOREIGN_INUSE_NOROUND;
+               } else {
+                       $connFreeKey = self::KEY_FOREIGN_FREE;
+                       $connInUseKey = self::KEY_FOREIGN_INUSE;
+               }
+
                $domain = $conn->getDomainID();
-               if ( !isset( $this->mConns[self::KEY_FOREIGN_INUSE][$serverIndex][$domain] ) ) {
+               if ( !isset( $this->mConns[$connInUseKey][$serverIndex][$domain] ) ) {
                        throw new InvalidArgumentException( __METHOD__ .
                                ": connection $serverIndex/$domain not found; it may have already been freed." );
-               } elseif ( $this->mConns[self::KEY_FOREIGN_INUSE][$serverIndex][$domain] !== $conn ) {
+               } elseif ( $this->mConns[$connInUseKey][$serverIndex][$domain] !== $conn ) {
                        throw new InvalidArgumentException( __METHOD__ .
                                ": connection $serverIndex/$domain mismatched; it may have already been freed." );
                }
+
                $conn->setLBInfo( 'foreignPoolRefCount', --$refCount );
                if ( $refCount <= 0 ) {
-                       $this->mConns[self::KEY_FOREIGN_FREE][$serverIndex][$domain] = $conn;
-                       unset( $this->mConns[self::KEY_FOREIGN_INUSE][$serverIndex][$domain] );
-                       if ( !$this->mConns[self::KEY_FOREIGN_INUSE][$serverIndex] ) {
-                               unset( $this->mConns[ self::KEY_FOREIGN_INUSE ][$serverIndex] ); // clean up
+                       $this->mConns[$connFreeKey][$serverIndex][$domain] = $conn;
+                       unset( $this->mConns[$connInUseKey][$serverIndex][$domain] );
+                       if ( !$this->mConns[$connInUseKey][$serverIndex] ) {
+                               unset( $this->mConns[$connInUseKey][$serverIndex] ); // clean up
                        }
                        $this->connLogger->debug( __METHOD__ . ": freed connection $serverIndex/$domain" );
                } else {
@@ -729,33 +745,26 @@ class LoadBalancer implements ILoadBalancer {
                }
        }
 
-       public function getConnectionRef( $db, $groups = [], $domain = false ) {
+       public function getConnectionRef( $db, $groups = [], $domain = false, $flags = 0 ) {
                $domain = ( $domain !== false ) ? $domain : $this->localDomain;
 
-               return new DBConnRef( $this, $this->getConnection( $db, $groups, $domain ) );
+               return new DBConnRef( $this, $this->getConnection( $db, $groups, $domain, $flags ) );
        }
 
-       public function getLazyConnectionRef( $db, $groups = [], $domain = false ) {
+       public function getLazyConnectionRef( $db, $groups = [], $domain = false, $flags = 0 ) {
                $domain = ( $domain !== false ) ? $domain : $this->localDomain;
 
-               return new DBConnRef( $this, [ $db, $groups, $domain ] );
+               return new DBConnRef( $this, [ $db, $groups, $domain, $flags ] );
        }
 
-       public function getMaintenanceConnectionRef( $db, $groups = [], $domain = false ) {
+       public function getMaintenanceConnectionRef( $db, $groups = [], $domain = false, $flags = 0 ) {
                $domain = ( $domain !== false ) ? $domain : $this->localDomain;
 
-               return new MaintainableDBConnRef( $this, $this->getConnection( $db, $groups, $domain ) );
+               return new MaintainableDBConnRef(
+                       $this, $this->getConnection( $db, $groups, $domain, $flags ) );
        }
 
-       /**
-        * @see ILoadBalancer::openConnection()
-        *
-        * @param int $i
-        * @param bool $domain
-        * @return bool|Database
-        * @throws DBAccessError
-        */
-       public function openConnection( $i, $domain = false ) {
+       public function openConnection( $i, $domain = false, $flags = 0 ) {
                if ( $this->localDomain->equals( $domain ) || $domain === $this->localDomainIdAlias ) {
                        $domain = false; // local connection requested
                }
@@ -767,26 +776,38 @@ class LoadBalancer implements ILoadBalancer {
                        $this->chronProt->initLB( $this );
                }
 
+               // Check if an auto-commit connection is being requested. If so, it will not reuse the
+               // 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 );
+
                if ( $domain !== false ) {
-                       $conn = $this->openForeignConnection( $i, $domain );
-               } elseif ( isset( $this->mConns[self::KEY_LOCAL][$i][0] ) ) {
-                       $conn = $this->mConns[self::KEY_LOCAL][$i][0];
+                       // Connection is to a foreign domain
+                       $conn = $this->openForeignConnection( $i, $domain, $flags );
                } else {
-                       if ( !isset( $this->mServers[$i] ) || !is_array( $this->mServers[$i] ) ) {
-                               throw new InvalidArgumentException( "No server with index '$i'." );
-                       }
-                       // Open a new connection
-                       $server = $this->mServers[$i];
-                       $server['serverIndex'] = $i;
-                       $conn = $this->reallyOpenConnection( $server, false );
-                       $serverName = $this->getServerName( $i );
-                       if ( $conn->isOpen() ) {
-                               $this->connLogger->debug( "Connected to database $i at '$serverName'." );
-                               $this->mConns[self::KEY_LOCAL][$i][0] = $conn;
+                       // Connection is to the local domain
+                       $connKey = $autoCommit ? self::KEY_LOCAL_NOROUND : self::KEY_LOCAL;
+                       if ( isset( $this->mConns[$connKey][$i][0] ) ) {
+                               $conn = $this->mConns[$connKey][$i][0];
                        } else {
-                               $this->connLogger->warning( "Failed to connect to database $i at '$serverName'." );
-                               $this->errorConnection = $conn;
-                               $conn = false;
+                               if ( !isset( $this->mServers[$i] ) || !is_array( $this->mServers[$i] ) ) {
+                                       throw new InvalidArgumentException( "No server with index '$i'." );
+                               }
+                               // Open a new connection
+                               $server = $this->mServers[$i];
+                               $server['serverIndex'] = $i;
+                               $server['autoCommitOnly'] = $autoCommit;
+                               $conn = $this->reallyOpenConnection( $server, false );
+                               $host = $this->getServerName( $i );
+                               if ( $conn->isOpen() ) {
+                                       $this->connLogger->debug( "Connected to database $i at '$host'." );
+                                       $this->mConns[$connKey][$i][0] = $conn;
+                               } else {
+                                       $this->connLogger->warning( "Failed to connect to database $i at '$host'." );
+                                       $this->errorConnection = $conn;
+                                       $conn = false;
+                               }
                        }
                }
 
@@ -799,6 +820,10 @@ class LoadBalancer implements ILoadBalancer {
                        $conn = false;
                }
 
+               if ( $autoCommit && $conn instanceof IDatabase ) {
+                       $conn->clearFlag( $conn::DBO_TRX ); // auto-commit mode
+               }
+
                return $conn;
        }
 
@@ -820,27 +845,37 @@ class LoadBalancer implements ILoadBalancer {
         *
         * @param int $i Server index
         * @param string $domain Domain ID to open
+        * @param integer $flags Class CONN_* constant bitfield
         * @return Database
         */
-       private function openForeignConnection( $i, $domain ) {
+       private function openForeignConnection( $i, $domain, $flags = 0 ) {
                $domainInstance = DatabaseDomain::newFromId( $domain );
                $dbName = $domainInstance->getDatabase();
                $prefix = $domainInstance->getTablePrefix();
+               $autoCommit = ( ( $flags & self::CONN_TRX_AUTO ) == self::CONN_TRX_AUTO );
+
+               if ( $autoCommit ) {
+                       $connFreeKey = self::KEY_FOREIGN_FREE_NOROUND;
+                       $connInUseKey = self::KEY_FOREIGN_INUSE_NOROUND;
+               } else {
+                       $connFreeKey = self::KEY_FOREIGN_FREE;
+                       $connInUseKey = self::KEY_FOREIGN_INUSE;
+               }
 
-               if ( isset( $this->mConns[self::KEY_FOREIGN_INUSE][$i][$domain] ) ) {
-                       // Reuse an in-use connection for the same domain that is not in-use
-                       $conn = $this->mConns[self::KEY_FOREIGN_INUSE][$i][$domain];
+               if ( isset( $this->mConns[$connInUseKey][$i][$domain] ) ) {
+                       // Reuse an in-use connection for the same domain
+                       $conn = $this->mConns[$connInUseKey][$i][$domain];
                        $this->connLogger->debug( __METHOD__ . ": reusing connection $i/$domain" );
-               } elseif ( isset( $this->mConns[self::KEY_FOREIGN_FREE][$i][$domain] ) ) {
-                       // Reuse a free connection for the same domain that is not in-use
-                       $conn = $this->mConns[self::KEY_FOREIGN_FREE][$i][$domain];
-                       unset( $this->mConns[self::KEY_FOREIGN_FREE][$i][$domain] );
-                       $this->mConns[self::KEY_FOREIGN_INUSE][$i][$domain] = $conn;
+               } elseif ( isset( $this->mConns[$connFreeKey][$i][$domain] ) ) {
+                       // Reuse a free connection for the same domain
+                       $conn = $this->mConns[$connFreeKey][$i][$domain];
+                       unset( $this->mConns[$connFreeKey][$i][$domain] );
+                       $this->mConns[$connInUseKey][$i][$domain] = $conn;
                        $this->connLogger->debug( __METHOD__ . ": reusing free connection $i/$domain" );
-               } elseif ( !empty( $this->mConns[self::KEY_FOREIGN_FREE][$i] ) ) {
-                       // Reuse a connection from another domain
-                       $conn = reset( $this->mConns[self::KEY_FOREIGN_FREE][$i] );
-                       $oldDomain = key( $this->mConns[self::KEY_FOREIGN_FREE][$i] );
+               } elseif ( !empty( $this->mConns[$connFreeKey][$i] ) ) {
+                       // Reuse a free connection from another domain
+                       $conn = reset( $this->mConns[$connFreeKey][$i] );
+                       $oldDomain = key( $this->mConns[$connFreeKey][$i] );
                        // The empty string as a DB name means "don't care".
                        // DatabaseMysqlBase::open() already handle this on connection.
                        if ( strlen( $dbName ) && !$conn->selectDB( $dbName ) ) {
@@ -850,8 +885,8 @@ class LoadBalancer implements ILoadBalancer {
                                $conn = false;
                        } else {
                                $conn->tablePrefix( $prefix );
-                               unset( $this->mConns[self::KEY_FOREIGN_FREE][$i][$oldDomain] );
-                               $this->mConns[self::KEY_FOREIGN_INUSE][$i][$domain] = $conn;
+                               unset( $this->mConns[$connFreeKey][$i][$oldDomain] );
+                               $this->mConns[$connInUseKey][$i][$domain] = $conn;
                                $this->connLogger->debug( __METHOD__ .
                                        ": reusing free connection from $oldDomain for $domain" );
                        }
@@ -864,6 +899,7 @@ class LoadBalancer implements ILoadBalancer {
                        $server['serverIndex'] = $i;
                        $server['foreignPoolRefCount'] = 0;
                        $server['foreign'] = true;
+                       $server['autoCommitOnly'] = $autoCommit;
                        $conn = $this->reallyOpenConnection( $server, $dbName );
                        if ( !$conn->isOpen() ) {
                                $this->connLogger->warning( __METHOD__ . ": connection error for $i/$domain" );
@@ -871,7 +907,7 @@ class LoadBalancer implements ILoadBalancer {
                                $conn = false;
                        } else {
                                $conn->tablePrefix( $prefix );
-                               $this->mConns[self::KEY_FOREIGN_INUSE][$i][$domain] = $conn;
+                               $this->mConns[$connInUseKey][$i][$domain] = $conn;
                                $this->connLogger->debug( __METHOD__ . ": opened new connection for $i/$domain" );
                        }
                }
@@ -1030,6 +1066,10 @@ class LoadBalancer implements ILoadBalancer {
                return ( $name != '' ) ? $name : 'localhost';
        }
 
+       public function getServerType( $i ) {
+               return isset( $this->mServers[$i]['type'] ) ? $this->mServers[$i]['type'] : 'unknown';
+       }
+
        /**
         * @deprecated Since 1.30, no alternative
         */
@@ -1083,8 +1123,11 @@ class LoadBalancer implements ILoadBalancer {
 
                $this->mConns = [
                        self::KEY_LOCAL => [],
-                       self::KEY_FOREIGN_FREE => [],
                        self::KEY_FOREIGN_INUSE => [],
+                       self::KEY_FOREIGN_FREE => [],
+                       self::KEY_LOCAL_NOROUND => [],
+                       self::KEY_FOREIGN_INUSE_NOROUND => [],
+                       self::KEY_FOREIGN_FREE_NOROUND => []
                ];
                $this->connsOpened = 0;
        }
@@ -1304,6 +1347,10 @@ class LoadBalancer implements ILoadBalancer {
         * @param IDatabase $conn
         */
        private function applyTransactionRoundFlags( IDatabase $conn ) {
+               if ( $conn->getLBInfo( 'autoCommitOnly' ) ) {
+                       return; // transaction rounds do not apply to these connections
+               }
+
                if ( $conn->getFlag( $conn::DBO_DEFAULT ) ) {
                        // 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.
@@ -1318,6 +1365,10 @@ class LoadBalancer implements ILoadBalancer {
         * @param IDatabase $conn
         */
        private function undoTransactionRoundFlags( IDatabase $conn ) {
+               if ( $conn->getLBInfo( 'autoCommitOnly' ) ) {
+                       return; // transaction rounds do not apply to these connections
+               }
+
                if ( $conn->getFlag( $conn::DBO_DEFAULT ) ) {
                        $conn->restoreFlags( $conn::RESTORE_PRIOR );
                }
index 3cce530..2cfd2a1 100644 (file)
@@ -145,27 +145,11 @@ class SqlBagOStuff extends BagOStuff {
                $this->replicaOnly = !empty( $params['slaveOnly'] );
        }
 
-       protected function getSeparateMainLB() {
-               global $wgDBtype;
-
-               if ( $this->usesMainDB() && $wgDBtype !== 'sqlite' ) {
-                       if ( !$this->separateMainLB ) {
-                               // We must keep a separate connection to MySQL in order to avoid deadlocks
-                               $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory();
-                               $this->separateMainLB = $lbFactory->newMainLB();
-                       }
-                       return $this->separateMainLB;
-               } else {
-                       // However, SQLite has an opposite behavior due to DB-level locking
-                       return null;
-               }
-       }
-
        /**
         * Get a connection to the specified database
         *
         * @param int $serverIndex
-        * @return IDatabase
+        * @return Database
         * @throws MWException
         */
        protected function getDB( $serverIndex ) {
@@ -181,8 +165,8 @@ class SqlBagOStuff extends BagOStuff {
                                throw $this->connFailureErrors[$serverIndex];
                        }
 
-                       # If server connection info was given, use that
                        if ( $this->serverInfos ) {
+                               // Use custom database defined by server connection info
                                $info = $this->serverInfos[$serverIndex];
                                $type = isset( $info['type'] ) ? $info['type'] : 'mysql';
                                $host = isset( $info['host'] ) ? $info['host'] : '[unknown]';
@@ -190,17 +174,22 @@ class SqlBagOStuff extends BagOStuff {
                                // Use a blank trx profiler to ignore expections as this is a cache
                                $info['trxProfiler'] = new TransactionProfiler();
                                $db = Database::factory( $type, $info );
-                               $db->clearFlag( DBO_TRX );
+                               $db->clearFlag( DBO_TRX ); // auto-commit mode
                        } else {
+                               // Use the main LB database
+                               $lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
                                $index = $this->replicaOnly ? DB_REPLICA : DB_MASTER;
-                               if ( $this->getSeparateMainLB() ) {
-                                       $db = $this->getSeparateMainLB()->getConnection( $index );
-                                       $db->clearFlag( DBO_TRX ); // auto-commit mode
+                               if ( $lb->getServerType( $lb->getWriterIndex() ) !== 'sqlite' ) {
+                                       // Keep a separate connection to avoid contention and deadlocks
+                                       $db = $lb->getConnection( $index, [], false, $lb::CONN_TRX_AUTO );
+                                       // @TODO: Use a blank trx profiler to ignore expections as this is a cache
                                } else {
-                                       $db = wfGetDB( $index );
-                                       // Can't mess with transaction rounds (DBO_TRX) :(
+                                       // However, SQLite has the opposite behavior due to DB-level locking.
+                                       // Stock sqlite MediaWiki installs use a separate sqlite cache DB instead.
+                                       $db = $lb->getConnection( $index );
                                }
                        }
+
                        $this->logger->debug( sprintf( "Connection %s will be used for SqlBagOStuff", $db ) );
                        $this->conns[$serverIndex] = $db;
                }
@@ -812,9 +801,7 @@ class SqlBagOStuff extends BagOStuff {
                        return true;
                }
 
-               $lb = $this->getSeparateMainLB()
-                       ?: MediaWikiServices::getInstance()->getDBLoadBalancer();
-
+               $lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
                if ( $lb->getServerCount() <= 1 ) {
                        return true; // no replica DBs
                }
diff --git a/tests/phpunit/includes/db/LoadBalancerTest.php b/tests/phpunit/includes/db/LoadBalancerTest.php
new file mode 100644 (file)
index 0000000..f8ab7f4
--- /dev/null
@@ -0,0 +1,135 @@
+<?php
+
+use Wikimedia\Rdbms\LoadBalancer;
+
+/**
+ * Holds tests for LoadBalancer MediaWiki class.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @group Database
+ * @file
+ */
+class LoadBalancerTest extends MediaWikiTestCase {
+       public function testLBSimpleServer() {
+               global $wgDBserver, $wgDBname, $wgDBuser, $wgDBpassword, $wgDBtype, $wgSQLiteDataDir;
+
+               $servers = [
+                       [
+                               'host'        => $wgDBserver,
+                               'dbname'      => $wgDBname,
+                               'user'        => $wgDBuser,
+                               'password'    => $wgDBpassword,
+                               'type'        => $wgDBtype,
+                               'dbDirectory' => $wgSQLiteDataDir,
+                               'load'        => 0,
+                               'flags'       => DBO_TRX // REPEATABLE-READ for consistency
+                       ],
+               ];
+
+               $lb = new LoadBalancer( [
+                       'servers' => $servers,
+                       'localDomain' => wfWikiID()
+               ] );
+
+               $dbw = $lb->getConnection( DB_MASTER );
+               $this->assertTrue( $dbw->getLBInfo( 'master' ), 'master shows as master' );
+               $this->assertTrue( $dbw->getFlag( $dbw::DBO_TRX ), "DBO_TRX set on master" );
+
+               $dbr = $lb->getConnection( DB_REPLICA );
+               $this->assertTrue( $dbr->getLBInfo( 'master' ), 'DB_REPLICA also gets the master' );
+               $this->assertTrue( $dbw->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" );
+               $this->assertTrue( $dbw->getFlag( $dbw::DBO_TRX ), "DBO_TRX still set on master" );
+               $this->assertNotEquals( $dbw, $dbwAuto, "CONN_TRX_AUTO 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" );
+               $this->assertTrue( $dbr->getFlag( $dbw::DBO_TRX ), "DBO_TRX still set on replica" );
+               $this->assertNotEquals( $dbr, $dbrAuto, "CONN_TRX_AUTO uses separate connection" );
+
+               $dbwAuto2 = $lb->getConnection( DB_MASTER, [], false, $lb::CONN_TRX_AUTO );
+               $this->assertEquals( $dbwAuto2, $dbwAuto, "CONN_TRX_AUTO reuses connections" );
+
+               $lb->closeAll();
+       }
+
+       public function testLBSimpleServers() {
+               global $wgDBserver, $wgDBname, $wgDBuser, $wgDBpassword, $wgDBtype, $wgSQLiteDataDir;
+
+               $servers = [
+                       [ // master
+                               'host'        => $wgDBserver,
+                               'dbname'      => $wgDBname,
+                               'user'        => $wgDBuser,
+                               'password'    => $wgDBpassword,
+                               'type'        => $wgDBtype,
+                               'dbDirectory' => $wgSQLiteDataDir,
+                               'load'        => 0,
+                               'flags'       => DBO_TRX // REPEATABLE-READ for consistency
+                       ],
+                       [ // emulated slave
+                               'host'        => $wgDBserver,
+                               'dbname'      => $wgDBname,
+                               'user'        => $wgDBuser,
+                               'password'    => $wgDBpassword,
+                               'type'        => $wgDBtype,
+                               'dbDirectory' => $wgSQLiteDataDir,
+                               'load'        => 100,
+                               'flags'       => DBO_TRX // REPEATABLE-READ for consistency
+                       ]
+               ];
+
+               $lb = new LoadBalancer( [
+                       'servers' => $servers,
+                       'localDomain' => wfWikiID(),
+                       'loadMonitorClass' => 'LoadMonitorNull'
+               ] );
+
+               $dbw = $lb->getConnection( DB_MASTER );
+               $this->assertTrue( $dbw->getLBInfo( 'master' ), 'master shows as master' );
+               $this->assertEquals(
+                       ( $wgDBserver != '' ) ? $wgDBserver : 'localhost',
+                       $dbw->getLBInfo( 'clusterMasterHost' ),
+                       'cluster master set' );
+               $this->assertTrue( $dbw->getFlag( $dbw::DBO_TRX ), "DBO_TRX set on master" );
+
+               $dbr = $lb->getConnection( DB_REPLICA );
+               $this->assertTrue( $dbr->getLBInfo( 'replica' ), 'slave shows as slave' );
+               $this->assertEquals(
+                       ( $wgDBserver != '' ) ? $wgDBserver : 'localhost',
+                       $dbr->getLBInfo( 'clusterMasterHost' ),
+                       'cluster master set' );
+               $this->assertTrue( $dbw->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" );
+               $this->assertTrue( $dbw->getFlag( $dbw::DBO_TRX ), "DBO_TRX still set on master" );
+               $this->assertNotEquals( $dbw, $dbwAuto, "CONN_TRX_AUTO 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" );
+               $this->assertTrue( $dbr->getFlag( $dbw::DBO_TRX ), "DBO_TRX still set on replica" );
+               $this->assertNotEquals( $dbr, $dbrAuto, "CONN_TRX_AUTO uses separate connection" );
+
+               $dbwAuto2 = $lb->getConnection( DB_MASTER, [], false, $lb::CONN_TRX_AUTO );
+               $this->assertEquals( $dbwAuto2, $dbwAuto, "CONN_TRX_AUTO reuses connections" );
+
+               $lb->closeAll();
+       }
+}