rdbms: clean up $groups logic in LoadBalancer and expand comments
authorAaron Schulz <aschulz@wikimedia.org>
Mon, 24 Jun 2019 23:36:42 +0000 (16:36 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Thu, 4 Jul 2019 22:26:55 +0000 (22:26 +0000)
Split out private resolveGroups() method for normalizing query
groups. Move some server index validation to getConnectionIndex()
and make it stricter.

Make getConnectionIndex() group fallback logic aware of the generic
group. The main use case for custom default groups is heavy scripts
or jobs that run in the background. It is probably better to have
good DB server redundancy for the query group and rely on it rather
than possibly fallback onto all of the main servers used for normal
requests. The later behavior could spread slowness or outages.

Also make getAnyOpenConnection() more robust and readable by
splitting out a private pickAnyOpenConnection() method that
checks IDatabase::trxLevel().

In addition, remove redundant is_int() check from isOpen()
method as it will return false in that case even without it.

Remove bogus getConnection() parameter in testCopyTestData().

Change-Id: Ica619c5487c761c724791d151db7388e4b41b0aa

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 8af6bb3..fe3d437 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 9641802..24f815d 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 );