Merge "rdbms: clean up $groups logic in LoadBalancer and expand comments"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Fri, 5 Jul 2019 22:00:09 +0000 (22:00 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Fri, 5 Jul 2019 22:00:09 +0000 (22:00 +0000)
includes/libs/rdbms/database/DBConnRef.php
includes/libs/rdbms/loadbalancer/ILoadBalancer.php
includes/libs/rdbms/loadbalancer/LoadBalancer.php
tests/phpunit/includes/db/LoadBalancerTest.php
tests/phpunit/tests/MediaWikiTestCaseTest.php

index d80a718..76701f8 100644 (file)
@@ -5,8 +5,23 @@ namespace Wikimedia\Rdbms;
 use InvalidArgumentException;
 
 /**
- * Helper class to handle automatically marking connections as reusable (via RAII pattern)
- * as well handling deferring the actual network connection until the handle is used
+ * Helper class used for automatically marking an IDatabase connection as reusable (once it no
+ * longer matters which DB domain is selected) and for deferring the actual network connection
+ *
+ * This uses an RAII-style pattern where calling code is expected to keep the returned reference
+ * handle as a function variable that falls out of scope when no longer needed. This avoids the
+ * need for matching reuseConnection() calls for every "return" statement as well as the tedious
+ * use of try/finally.
+ *
+ * @par Example:
+ * @code
+ *     function getRowData() {
+ *         $conn = $this->lb->getConnectedRef( DB_REPLICA );
+ *         $row = $conn->select( ... );
+ *         return $row ? (array)$row : false;
+ *         // $conn falls out of scope and $this->lb->reuseConnection() gets called
+ *     }
+ * @endcode
  *
  * @ingroup Database
  * @since 1.22
index b086beb..2f9857f 100644 (file)
@@ -23,6 +23,7 @@
 namespace Wikimedia\Rdbms;
 
 use Exception;
+use LogicException;
 use InvalidArgumentException;
 
 /**
@@ -85,6 +86,8 @@ interface ILoadBalancer {
 
        /** @var string Domain specifier when no specific database needs to be selected */
        const DOMAIN_ANY = '';
+       /** @var bool The generic query group (bool gives b/c with 1.33 method signatures) */
+       const GROUP_GENERIC = false;
 
        /** @var int DB handle should have DBO_TRX disabled and the caller will leave it as such */
        const CONN_TRX_AUTOCOMMIT = 1;
@@ -100,25 +103,26 @@ interface ILoadBalancer {
         * Construct a manager of IDatabase connection objects
         *
         * @param array $params Parameter map with keys:
-        *  - servers : Required. Array of server info structures.
-        *  - localDomain: A DatabaseDomain or domain ID string.
-        *  - loadMonitor : Name of a class used to fetch server lag and load.
+        *  - servers : List of server info structures
+        *  - localDomain: A DatabaseDomain or domain ID string
+        *  - loadMonitor : Name of a class used to fetch server lag and load
         *  - readOnlyReason : Reason the master DB is read-only if so [optional]
         *  - waitTimeout : Maximum time to wait for replicas for consistency [optional]
         *  - maxLag: Try to avoid DB replicas with lag above this many seconds [optional]
         *  - srvCache : BagOStuff object for server cache [optional]
         *  - wanCache : WANObjectCache object [optional]
         *  - chronologyCallback: Callback to run before the first connection attempt [optional]
+        *  - defaultGroup: Default query group; the generic group if not specified [optional]
         *  - hostname : The name of the current server [optional]
-        *  - cliMode: Whether the execution context is a CLI script. [optional]
-        *  - profiler : Class name or instance with profileIn()/profileOut() methods. [optional]
-        *  - trxProfiler: TransactionProfiler instance. [optional]
-        *  - replLogger: PSR-3 logger instance. [optional]
-        *  - connLogger: PSR-3 logger instance. [optional]
-        *  - queryLogger: PSR-3 logger instance. [optional]
-        *  - perfLogger: PSR-3 logger instance. [optional]
-        *  - errorLogger : Callback that takes an Exception and logs it. [optional]
-        *  - deprecationLogger: Callback to log a deprecation warning. [optional]
+        *  - cliMode: Whether the execution context is a CLI script [optional]
+        *  - profiler : Class name or instance with profileIn()/profileOut() methods [optional]
+        *  - trxProfiler: TransactionProfiler instance [optional]
+        *  - replLogger: PSR-3 logger instance [optional]
+        *  - connLogger: PSR-3 logger instance [optional]
+        *  - queryLogger: PSR-3 logger instance [optional]
+        *  - perfLogger: PSR-3 logger instance [optional]
+        *  - errorLogger : Callback that takes an Exception and logs it [optional]
+        *  - deprecationLogger: Callback to log a deprecation warning [optional]
         *  - roundStage: STAGE_POSTCOMMIT_* class constant; for internal use [optional]
         *  - ownerId: integer ID of an LBFactory instance that manages this instance [optional]
         * @throws InvalidArgumentException
@@ -159,9 +163,9 @@ interface ILoadBalancer {
         * Subsequent calls with the same $group will not need to make new connection attempts
         * since the acquired connection for each group is preserved.
         *
-        * @param string|bool $group Query group, or false for the generic group
-        * @param string|bool $domain Domain ID, or false for the current domain
-        * @throws DBError
+        * @param string|bool $group Query group or false for the generic group
+        * @param string|bool $domain DB domain ID or false for the local domain
+        * @throws DBError If no live handle can be obtained
         * @return bool|int|string
         */
        public function getReaderIndex( $group = false, $domain = false );
@@ -204,7 +208,8 @@ interface ILoadBalancer {
         * Get any open connection to a given server index, local or foreign
         *
         * Use CONN_TRX_AUTOCOMMIT to only look for connections opened with that flag.
-        * Avoid the use of begin() or startAtomic() on any such connections.
+        * Avoid the use of transaction methods like IDatabase::begin() or IDatabase::startAtomic()
+        * on any such connections.
         *
         * @param int $i Server index or DB_MASTER/DB_REPLICA
         * @param int $flags Bitfield of CONN_* class constants
@@ -213,95 +218,136 @@ interface ILoadBalancer {
        public function getAnyOpenConnection( $i, $flags = 0 );
 
        /**
-        * Get a connection handle by server index
-        *
-        * 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_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.
-        * Avoid the use of begin() or startAtomic() on any such connections.
+        * Get a live handle for a real or virtual (DB_MASTER/DB_REPLICA) server index
+        *
+        * The server index, $i, can be one of the following:
+        *   - DB_REPLICA: a server index will be selected by the load balancer based on read
+        *      weight, connectivity, and replication lag. Note that the master server might be
+        *      configured with read weight. If $groups is empty then it means "the generic group",
+        *      in which case all servers defined with read weight will be considered. Additional
+        *      query groups can be configured, having their own list of server indexes and read
+        *      weights. If a query group list is provided in $groups, then each recognized group
+        *      will be tried, left-to-right, until server index selection succeeds or all groups
+        *      have been tried, in which case the generic group will be tried.
+        *   - DB_MASTER: the master server index will be used; the same as getWriterIndex().
+        *      The value of $groups should be [] when using this server index.
+        *   - Specific server index: a positive integer can be provided to use the server with
+        *      that index. An error will be thrown in no such server index is recognized. This
+        *      server selection method is usually only useful for internal load balancing logic.
+        *      The value of $groups should be [] when using a specific server index.
+        *
+        * Handles acquired by this method, getConnectionRef(), getLazyConnectionRef(), and
+        * getMaintenanceConnectionRef() use the same set of shared connection pools. Callers that
+        * get a *local* DB domain handle for the same server will share one handle for all of those
+        * callers using CONN_TRX_AUTOCOMMIT (via $flags) and one handle for all of those callers not
+        * using CONN_TRX_AUTOCOMMIT. Callers that get a *foreign* DB domain handle (via $domain) will
+        * share any handle that has the right CONN_TRX_AUTOCOMMIT mode and is already on the right
+        * DB domain. Otherwise, one of the "free for reuse" handles will be claimed or a new handle
+        * will be made if there are none.
+        *
+        * Handle sharing is particularly useful when callers get local DB domain (the default),
+        * transaction round aware (the default), DB_MASTER handles. All such callers will operate
+        * within a single database transaction as a consequence. Handle sharing is also useful when
+        * callers get local DB domain (the default), transaction round aware (the default), samely
+        * query grouped (the default), DB_REPLICA handles. All such callers will operate within a
+        * single database transaction as a consequence.
+        *
+        * Calling functions that use $domain must call reuseConnection() once the last query of the
+        * function is executed. This lets the load balancer share this handle with other callers
+        * requesting connections on different database domains.
+        *
+        * Use CONN_TRX_AUTOCOMMIT to use a separate pool of only auto-commit handles. This flag
+        * is ignored for databases with ATTR_DB_LEVEL_LOCKING (e.g. sqlite) in order to avoid
+        * deadlocks. getServerAttributes() can be used to check such attributes beforehand. Avoid
+        * using IDatabase::begin() and IDatabase::commit() on such handles. If it is not possible
+        * to avoid using methods like IDatabase::startAtomic() and IDatabase::endAtomic(), callers
+        * should at least make sure that the atomic sections are closed on failure via try/catch
+        * and IDatabase::cancelAtomic().
+        *
+        * @see ILoadBalancer::reuseConnection()
+        * @see ILoadBalancer::getServerAttributes()
         *
         * @param int $i Server index (overrides $groups) 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 string[]|string $groups Query group(s) or [] to use the default group
+        * @param string|bool $domain DB domain ID or false for the local domain
         * @param int $flags Bitfield of CONN_* class constants
-        *
-        * @note This method throws DBAccessError if ILoadBalancer::disable() was called
-        *
-        * @throws DBError If any error occurs that prevents the yielding of a (live) IDatabase
-        * @return IDatabase|bool This returns false on failure if CONN_SILENCE_ERRORS is set
+        * @return IDatabase|bool Live connection handle or false on failure
+        * @throws DBError If no live handle can be obtained and CONN_SILENCE_ERRORS is not set
+        * @throws DBAccessError If disable() was previously called
         */
        public function getConnection( $i, $groups = [], $domain = false, $flags = 0 );
 
        /**
-        * Mark a foreign connection as being available for reuse under a different DB domain
+        * Mark a live handle as being available for reuse under a different database domain
+        *
+        * This mechanism is reference-counted, and must be called the same number of times as
+        * getConnection() to work. Never call this on handles acquired via getConnectionRef(),
+        * getLazyConnectionRef(), and getMaintenanceConnectionRef(), as they already manage
+        * the logic of calling this method when they fall out of scope in PHP.
         *
-        * This mechanism is reference-counted, and must be called the same number of times
-        * as getConnection() to work.
+        * @see ILoadBalancer::getConnection()
         *
         * @param IDatabase $conn
-        * @throws InvalidArgumentException
+        * @throws LogicException
         */
        public function reuseConnection( IDatabase $conn );
 
        /**
-        * Get a database connection handle reference
-        *
-        * The handle's methods simply wrap those of a Database handle
+        * Get a live database handle reference for a real or virtual (DB_MASTER/DB_REPLICA) server index
         *
         * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING
-        * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes()
+        * (e.g. sqlite) in order to avoid deadlocks. getServerAttributes()
         * can be used to check such flags beforehand. Avoid the use of begin() or startAtomic()
         * on any CONN_TRX_AUTOCOMMIT connections.
         *
         * @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 string[]|string $groups Query group(s) or [] to use the default group
+        * @param string|bool $domain DB domain ID or false for the local domain
         * @param int $flags Bitfield of CONN_* class constants (e.g. CONN_TRX_AUTOCOMMIT)
         * @return DBConnRef
         */
        public function getConnectionRef( $i, $groups = [], $domain = false, $flags = 0 );
 
        /**
-        * Get a database connection handle reference without connecting yet
+        * Get a database handle reference for a real or virtual (DB_MASTER/DB_REPLICA) server index
         *
-        * The handle's methods simply wrap those of a Database handle
+        * The handle's methods simply proxy to those of an underlying IDatabase handle which
+        * takes care of the actual connection and query logic.
         *
         * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING
-        * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes()
+        * (e.g. sqlite) in order to avoid deadlocks. getServerAttributes()
         * can be used to check such flags beforehand. Avoid the use of begin() or startAtomic()
         * on any CONN_TRX_AUTOCOMMIT connections.
         *
         * @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 string[]|string $groups Query group(s) or [] to use the default group
+        * @param string|bool $domain DB domain ID or false for the local domain
         * @param int $flags Bitfield of CONN_* class constants (e.g. CONN_TRX_AUTOCOMMIT)
         * @return DBConnRef
         */
        public function getLazyConnectionRef( $i, $groups = [], $domain = false, $flags = 0 );
 
        /**
-        * Get a maintenance database connection handle reference for migrations and schema changes
+        * Get a live database handle for a real or virtual (DB_MASTER/DB_REPLICA) server index
+        * that can be used for data migrations and schema changes
         *
-        * The handle's methods simply wrap those of a Database handle
+        * The handle's methods simply proxy to those of an underlying IDatabase handle which
+        * takes care of the actual connection and query logic.
         *
         * The CONN_TRX_AUTOCOMMIT flag is ignored for databases with ATTR_DB_LEVEL_LOCKING
-        * (e.g. sqlite) in order to avoid deadlocks. ILoadBalancer::getServerAttributes()
+        * (e.g. sqlite) in order to avoid deadlocks. getServerAttributes()
         * can be used to check such flags beforehand. Avoid the use of begin() or startAtomic()
         * on any CONN_TRX_AUTOCOMMIT connections.
         *
         * @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 string[]|string $groups Query group(s) or [] to use the default group
+        * @param string|bool $domain DB domain ID or false for the local domain
         * @param int $flags Bitfield of CONN_* class constants (e.g. CONN_TRX_AUTOCOMMIT)
         * @return MaintainableDBConnRef
         */
@@ -362,7 +408,7 @@ interface ILoadBalancer {
        public function getServerName( $i );
 
        /**
-        * Return the server info structure for a given index, or false if the index is invalid.
+        * Return the server info structure for a given index or false if the index is invalid.
         * @param int $i
         * @return array|bool
         * @since 1.31
@@ -544,7 +590,7 @@ interface ILoadBalancer {
 
        /**
         * @note This method will trigger a DB connection if not yet done
-        * @param string|bool $domain Domain ID, or false for the current domain
+        * @param string|bool $domain DB domain ID or false for the local domain
         * @return bool Whether the database for generic connections this request is highly "lagged"
         */
        public function getLaggedReplicaMode( $domain = false );
@@ -561,7 +607,7 @@ interface ILoadBalancer {
 
        /**
         * @note This method may trigger a DB connection if not yet done
-        * @param string|bool $domain Domain ID, or false for the current domain
+        * @param string|bool $domain DB domain ID or false for the local domain
         * @param IDatabase|null $conn DB master connection; used to avoid loops [optional]
         * @return string|bool Reason the master is read-only or false if it is not
         */
@@ -607,7 +653,7 @@ interface ILoadBalancer {
         * May attempt to open connections to replica DBs on the default DB. If there is
         * no lag, the maximum lag will be reported as -1.
         *
-        * @param bool|string $domain Domain ID, or false for the default database
+        * @param bool|string $domain Domain ID or false for the default database
         * @return array ( host, max lag, index of max lagged host )
         */
        public function getMaxLag( $domain = false );
index 44d526c..b640dc0 100644 (file)
@@ -28,6 +28,7 @@ use BagOStuff;
 use EmptyBagOStuff;
 use WANObjectCache;
 use ArrayUtils;
+use LogicException;
 use UnexpectedValueException;
 use InvalidArgumentException;
 use RuntimeException;
@@ -84,8 +85,10 @@ class LoadBalancer implements ILoadBalancer {
        private $loadMonitorConfig;
        /** @var string Alternate local DB domain instead of DatabaseDomain::getId() */
        private $localDomainIdAlias;
-       /** @var int */
+       /** @var int Amount of replication lag, in seconds, that is considered "high" */
        private $maxLag;
+       /** @var string|bool The query group list to be used by default */
+       private $defaultGroup;
 
        /** @var string Current server name */
        private $hostname;
@@ -101,11 +104,11 @@ class LoadBalancer implements ILoadBalancer {
        /** @var array[] Map of (name => callable) */
        private $trxRecurringCallbacks = [];
 
-       /** @var Database DB connection object that caused a problem */
+       /** @var Database Connection handle that caused a problem */
        private $errorConnection;
-       /** @var int The generic (not query grouped) replica DB index */
+       /** @var int The generic (not query grouped) replica server index */
        private $genericReadIndex = -1;
-       /** @var int[] The group replica DB indexes keyed by group */
+       /** @var int[] The group replica server indexes keyed by group */
        private $readIndexByGroup = [];
        /** @var bool|DBMasterPos Replication sync position or false if not set */
        private $waitForPos;
@@ -113,9 +116,9 @@ class LoadBalancer implements ILoadBalancer {
        private $laggedReplicaMode = false;
        /** @var bool Whether the generic reader fell back to a lagged replica DB */
        private $allReplicasDownMode = false;
-       /** @var string The last DB selection or connection error */
+       /** @var string The last DB domain selection or connection error */
        private $lastError = 'Unknown error';
-       /** @var string|bool Reason the LB is read-only or false if not */
+       /** @var string|bool Reason this instance is read-only or false if not */
        private $readOnlyReason = false;
        /** @var int Total number of new connections ever made with this instance */
        private $connectionCounter = 0;
@@ -131,9 +134,6 @@ class LoadBalancer implements ILoadBalancer {
        /** @var string Stage of the current transaction round in the transaction round life-cycle */
        private $trxRoundStage = self::ROUND_CURSORY;
 
-       /** @var string|null */
-       private $defaultGroup = null;
-
        /** @var int Warn when this many connection are held */
        const CONN_HELD_WARN_THRESHOLD = 10;
 
@@ -244,7 +244,7 @@ class LoadBalancer implements ILoadBalancer {
                        }
                }
 
-               $this->defaultGroup = $params['defaultGroup'] ?? null;
+               $this->defaultGroup = $params['defaultGroup'] ?? self::GROUP_GENERIC;
                $this->ownerId = $params['ownerId'] ?? null;
        }
 
@@ -274,6 +274,30 @@ class LoadBalancer implements ILoadBalancer {
                return (string)$domain;
        }
 
+       /**
+        * @param string[]|string|bool $groups Query group list or false for the default
+        * @param int $i Specific server index or DB_MASTER/DB_REPLICA
+        * @return string[]|bool[] Query group list
+        */
+       private function resolveGroups( $groups, $i ) {
+               if ( $groups === false ) {
+                       $resolvedGroups = [ $this->defaultGroup ];
+               } elseif ( is_string( $groups ) ) {
+                       $resolvedGroups = [ $groups ];
+               } elseif ( is_array( $groups ) ) {
+                       $resolvedGroups = $groups ?: [ $this->defaultGroup ];
+               } else {
+                       throw new InvalidArgumentException( "Invalid query groups provided" );
+               }
+
+               if ( $groups && $i > 0 ) {
+                       $groupList = implode( ', ', $groups );
+                       throw new LogicException( "Got query groups ($groupList) with a server index (#$i)" );
+               }
+
+               return $resolvedGroups;
+       }
+
        /**
         * @param int $flags
         * @return bool
@@ -399,43 +423,42 @@ class LoadBalancer implements ILoadBalancer {
        }
 
        /**
-        * @param int $i
-        * @param array $groups
+        * Get the server index to use for a specified server index and query group list
+        *
+        * @param int $i Specific server index or DB_MASTER/DB_REPLICA
+        * @param string[]|bool[] $groups Resolved query group list (non-empty)
         * @param string|bool $domain
-        * @return int The index of a specific server (replica DBs are checked for connectivity)
+        * @return int A specific server index (replica DBs are checked for connectivity)
         */
-       private function getConnectionIndex( $i, $groups, $domain ) {
-               // Check one "group" per default: the generic pool
-               $defaultGroups = $this->defaultGroup ? [ $this->defaultGroup ] : [ false ];
-
-               $groups = ( $groups === false || $groups === [] )
-                       ? $defaultGroups
-                       : (array)$groups;
-
+       private function getConnectionIndex( $i, array $groups, $domain ) {
                if ( $i === self::DB_MASTER ) {
                        $i = $this->getWriterIndex();
                } elseif ( $i === self::DB_REPLICA ) {
-                       # Try to find an available server in any the query groups (in order)
+                       // Find an available server in any of the query groups (in order)
                        foreach ( $groups as $group ) {
                                $groupIndex = $this->getReaderIndex( $group, $domain );
                                if ( $groupIndex !== false ) {
-                                       $i = $groupIndex;
+                                       $i = $groupIndex; // group connection succeeded
                                        break;
                                }
                        }
+               } elseif ( !isset( $this->servers[$i] ) ) {
+                       throw new UnexpectedValueException( "Invalid server index index #$i" );
                }
 
-               # Operation-based index
                if ( $i === self::DB_REPLICA ) {
-                       $this->lastError = 'Unknown error'; // reset error string
-                       # Try the general server pool if $groups are unavailable.
-                       $i = ( $groups === [ false ] )
-                               ? false // don't bother with this if that is what was tried above
-                               : $this->getReaderIndex( false, $domain );
-                       # Couldn't find a working server in getReaderIndex()?
+                       // No specific server was yet found
+                       $this->lastError = 'Unknown error'; // set here in case of worse failure
+                       // Either make one last connection attempt or give up
+                       $i = in_array( $this->defaultGroup, $groups, true )
+                               // Connection attempt already included the default query group; give up
+                               ? false
+                               // Connection attempt was for other query groups; try the default one
+                               : $this->getReaderIndex( $this->defaultGroup, $domain );
+
                        if ( $i === false ) {
+                               // Still coundn't find a working non-zero read load server
                                $this->lastError = 'No working replica DB server: ' . $this->lastError;
-                               // Throw an exception
                                $this->reportConnectionError();
                                return null; // unreachable due to exception
                        }
@@ -456,7 +479,7 @@ class LoadBalancer implements ILoadBalancer {
                        return $index;
                }
 
-               if ( $group !== false ) {
+               if ( $group !== self::GROUP_GENERIC ) {
                        // Use the server weight array for this load group
                        if ( isset( $this->groupLoads[$group] ) ) {
                                $loads = $this->groupLoads[$group];
@@ -492,7 +515,7 @@ class LoadBalancer implements ILoadBalancer {
                // Cache the reader index for future DB_REPLICA handles
                $this->setExistingReaderIndex( $group, $i );
                // Record whether the generic reader index is in "lagged replica DB" mode
-               if ( $group === false && $laggedReplicaMode ) {
+               if ( $group === self::GROUP_GENERIC && $laggedReplicaMode ) {
                        $this->laggedReplicaMode = true;
                }
 
@@ -509,7 +532,7 @@ class LoadBalancer implements ILoadBalancer {
         * @return int Server index or -1 if none was chosen
         */
        protected function getExistingReaderIndex( $group ) {
-               if ( $group === false ) {
+               if ( $group === self::GROUP_GENERIC ) {
                        $index = $this->genericReadIndex;
                } else {
                        $index = $this->readIndexByGroup[$group] ?? -1;
@@ -529,7 +552,7 @@ class LoadBalancer implements ILoadBalancer {
                        throw new UnexpectedValueException( "Cannot set a negative read server index" );
                }
 
-               if ( $group === false ) {
+               if ( $group === self::GROUP_GENERIC ) {
                        $this->genericReadIndex = $index;
                } else {
                        $this->readIndexByGroup[$group] = $index;
@@ -704,7 +727,9 @@ class LoadBalancer implements ILoadBalancer {
                $i = ( $i === self::DB_MASTER ) ? $this->getWriterIndex() : $i;
                $autocommit = ( ( $flags & self::CONN_TRX_AUTOCOMMIT ) == self::CONN_TRX_AUTOCOMMIT );
 
+               $conn = false;
                foreach ( $this->conns as $connsByServer ) {
+                       // Get the connection array server indexes to inspect
                        if ( $i === self::DB_REPLICA ) {
                                $indexes = array_keys( $connsByServer );
                        } else {
@@ -712,18 +737,47 @@ class LoadBalancer implements ILoadBalancer {
                        }
 
                        foreach ( $indexes as $index ) {
-                               foreach ( $connsByServer[$index] as $conn ) {
-                                       if ( !$conn->isOpen() ) {
-                                               continue; // some sort of error occured?
-                                       }
-                                       if ( !$autocommit || $conn->getLBInfo( 'autoCommitOnly' ) ) {
-                                               return $conn;
-                                       }
+                               $conn = $this->pickAnyOpenConnection( $connsByServer[$index], $autocommit );
+                               if ( $conn ) {
+                                       break;
                                }
                        }
                }
 
-               return false;
+               if ( $conn ) {
+                       $this->enforceConnectionFlags( $conn, $flags );
+               }
+
+               return $conn;
+       }
+
+       /**
+        * @param IDatabase[] $candidateConns
+        * @param bool $autocommit Whether to only look for auto-commit connections
+        * @return IDatabase|false An appropriate open connection or false if none found
+        */
+       private function pickAnyOpenConnection( $candidateConns, $autocommit ) {
+               $conn = false;
+
+               foreach ( $candidateConns as $candidateConn ) {
+                       if ( !$candidateConn->isOpen() ) {
+                               continue; // some sort of error occured?
+                       } elseif (
+                               $autocommit &&
+                               (
+                                       // Connection is transaction round aware
+                                       !$candidateConn->getLBInfo( 'autoCommitOnly' ) ||
+                                       // Some sort of error left a transaction open?
+                                       $candidateConn->trxLevel()
+                               )
+                       ) {
+                               continue; // some sort of error left a transaction open?
+                       }
+
+                       $conn = $candidateConn;
+               }
+
+               return $conn;
        }
 
        /**
@@ -823,12 +877,7 @@ class LoadBalancer implements ILoadBalancer {
        }
 
        public function getConnection( $i, $groups = [], $domain = false, $flags = 0 ) {
-               if ( !is_int( $i ) ) {
-                       throw new InvalidArgumentException( "Cannot connect without a server index" );
-               } elseif ( $groups && $i > 0 ) {
-                       throw new InvalidArgumentException( "Got query groups with server index #$i" );
-               }
-
+               $groups = $this->resolveGroups( $groups, $i );
                $domain = $this->resolveDomainID( $domain );
                $flags = $this->sanitizeConnectionFlags( $flags );
                $masterOnly = ( $i === self::DB_MASTER || $i === $this->getWriterIndex() );
@@ -896,7 +945,7 @@ class LoadBalancer implements ILoadBalancer {
                        // Database instance to this method. Any caller passing in a DBConnRef is broken.
                        $this->connLogger->error(
                                __METHOD__ . ": got DBConnRef instance.\n" .
-                               ( new RuntimeException() )->getTraceAsString() );
+                               ( new LogicException() )->getTraceAsString() );
 
                        return;
                }
@@ -1154,14 +1203,9 @@ class LoadBalancer implements ILoadBalancer {
         * Test if the specified index represents an open connection
         *
         * @param int $index Server index
-        * @private
         * @return bool
         */
        private function isOpen( $index ) {
-               if ( !is_int( $index ) ) {
-                       return false;
-               }
-
                return (bool)$this->getAnyOpenConnection( $index );
        }
 
index 1645b85..defa0aa 100644 (file)
@@ -112,7 +112,7 @@ class LoadBalancerTest extends MediaWikiTestCase {
                global $wgDBserver;
 
                // Simulate web request with DBO_TRX
-               $lb = $this->newMultiServerLocalLoadBalancer( DBO_TRX );
+               $lb = $this->newMultiServerLocalLoadBalancer( [], [ 'flags' => DBO_TRX ] );
 
                $this->assertEquals( 8, $lb->getServerCount() );
                $this->assertTrue( $lb->hasReplicaServers() );
@@ -167,12 +167,12 @@ class LoadBalancerTest extends MediaWikiTestCase {
                ] );
        }
 
-       private function newMultiServerLocalLoadBalancer( $flags = DBO_DEFAULT ) {
+       private function newMultiServerLocalLoadBalancer( $lbExtra = [], $srvExtra = [] ) {
                global $wgDBserver, $wgDBname, $wgDBuser, $wgDBpassword, $wgDBtype, $wgSQLiteDataDir;
 
                $servers = [
                        // Master DB
-                       0 => [
+                       0 => $srvExtra + [
                                'host' => $wgDBserver,
                                'dbname' => $wgDBname,
                                'tablePrefix' => $this->dbPrefix(),
@@ -181,10 +181,9 @@ class LoadBalancerTest extends MediaWikiTestCase {
                                'type' => $wgDBtype,
                                'dbDirectory' => $wgSQLiteDataDir,
                                'load' => 0,
-                               'flags' => $flags
                        ],
                        // Main replica DBs
-                       1 => [
+                       1 => $srvExtra + [
                                'host' => $wgDBserver,
                                'dbname' => $wgDBname,
                                'tablePrefix' => $this->dbPrefix(),
@@ -193,9 +192,8 @@ class LoadBalancerTest extends MediaWikiTestCase {
                                'type' => $wgDBtype,
                                'dbDirectory' => $wgSQLiteDataDir,
                                'load' => 100,
-                               'flags' => $flags
                        ],
-                       2 => [
+                       2 => $srvExtra + [
                                'host' => $wgDBserver,
                                'dbname' => $wgDBname,
                                'tablePrefix' => $this->dbPrefix(),
@@ -204,10 +202,9 @@ class LoadBalancerTest extends MediaWikiTestCase {
                                'type' => $wgDBtype,
                                'dbDirectory' => $wgSQLiteDataDir,
                                'load' => 100,
-                               'flags' => $flags
                        ],
                        // RC replica DBs
-                       3 => [
+                       3 => $srvExtra + [
                                'host' => $wgDBserver,
                                'dbname' => $wgDBname,
                                'tablePrefix' => $this->dbPrefix(),
@@ -220,10 +217,9 @@ class LoadBalancerTest extends MediaWikiTestCase {
                                        'recentchanges' => 100,
                                        'watchlist' => 100
                                ],
-                               'flags' => $flags
                        ],
                        // Logging replica DBs
-                       4 => [
+                       4 => $srvExtra + [
                                'host' => $wgDBserver,
                                'dbname' => $wgDBname,
                                'tablePrefix' => $this->dbPrefix(),
@@ -235,9 +231,8 @@ class LoadBalancerTest extends MediaWikiTestCase {
                                'groupLoads' => [
                                        'logging' => 100
                                ],
-                               'flags' => $flags
                        ],
-                       5 => [
+                       5 => $srvExtra + [
                                'host' => $wgDBserver,
                                'dbname' => $wgDBname,
                                'tablePrefix' => $this->dbPrefix(),
@@ -249,10 +244,9 @@ class LoadBalancerTest extends MediaWikiTestCase {
                                'groupLoads' => [
                                        'logging' => 100
                                ],
-                               'flags' => $flags
                        ],
                        // Maintenance query replica DBs
-                       6 => [
+                       6 => $srvExtra + [
                                'host' => $wgDBserver,
                                'dbname' => $wgDBname,
                                'tablePrefix' => $this->dbPrefix(),
@@ -264,10 +258,9 @@ class LoadBalancerTest extends MediaWikiTestCase {
                                'groupLoads' => [
                                        'vslow' => 100
                                ],
-                               'flags' => $flags
                        ],
                        // Replica DB that only has a copy of some static tables
-                       7 => [
+                       7 => $srvExtra + [
                                'host' => $wgDBserver,
                                'dbname' => $wgDBname,
                                'tablePrefix' => $this->dbPrefix(),
@@ -279,12 +272,11 @@ class LoadBalancerTest extends MediaWikiTestCase {
                                'groupLoads' => [
                                        'archive' => 100
                                ],
-                               'flags' => $flags,
                                'is static' => true
                        ]
                ];
 
-               return new LoadBalancer( [
+               return new LoadBalancer( $lbExtra + [
                        'servers' => $servers,
                        'localDomain' => new DatabaseDomain( $wgDBname, null, $this->dbPrefix() ),
                        'queryLogger' => MediaWiki\Logger\LoggerFactory::getInstance( 'DBQuery' ),
@@ -585,7 +577,7 @@ class LoadBalancerTest extends MediaWikiTestCase {
        }
 
        public function testQueryGroupIndex() {
-               $lb = $this->newMultiServerLocalLoadBalancer();
+               $lb = $this->newMultiServerLocalLoadBalancer( [ 'defaultGroup' => false ] );
                /** @var LoadBalancer $lbWrapper */
                $lbWrapper = TestingAccessWrapper::newFromObject( $lb );
 
index 88fc93b..37fa030 100644 (file)
@@ -167,7 +167,7 @@ class MediaWikiTestCaseTest extends MediaWikiTestCase {
 
                $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory();
                $lb = $lbFactory->newMainLB();
-               $db = $lb->getConnection( DB_REPLICA, DBO_TRX );
+               $db = $lb->getConnection( DB_REPLICA );
 
                // sanity
                $this->assertNotSame( $this->db, $db );