Merge "rdbms: Support secondary autocommit connections in LoadBalancer"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Fri, 18 Aug 2017 01:38:01 +0000 (01:38 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Fri, 18 Aug 2017 01:38:01 +0000 (01:38 +0000)
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 56a7fbb..1665a5e 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();
+       }
+}