From 99c80a8fc7bb4d434ab5fad2d0ca404716d8f8db Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Wed, 19 Jul 2017 20:51:54 -0700 Subject: [PATCH] rdbms: Support secondary autocommit connections in LoadBalancer This is useful for things like SqlBagOStuff and JobQueueDB being able to write in auto-commit mode while the main transaction round still goes on. SqlBagOStuff already had this sort of logic. JobQueueDB now also takes this approach so it does not have to defer insertion except for sqlite. This makes callers more likely to know when insertion failed or not. Make sure LoadBalancer sets "master"/"replica" LB info itself; this fixes a bug found in the new tests. Bug: T140338 Bug: T42451 Change-Id: I4199a9598d7afbf976a6efa8ed84b85b56da02bd --- includes/jobqueue/JobQueueDB.php | 32 ++- includes/libs/rdbms/database/DBConnRef.php | 9 +- .../libs/rdbms/loadbalancer/ILoadBalancer.php | 51 +++-- .../libs/rdbms/loadbalancer/LoadBalancer.php | 187 +++++++++++------- includes/objectcache/SqlBagOStuff.php | 41 ++-- .../phpunit/includes/db/LoadBalancerTest.php | 135 +++++++++++++ 6 files changed, 334 insertions(+), 121 deletions(-) create mode 100644 tests/phpunit/includes/db/LoadBalancerTest.php diff --git a/includes/jobqueue/JobQueueDB.php b/includes/jobqueue/JobQueueDB.php index b7cc133a75..b5f331b35f 100644 --- a/includes/jobqueue/JobQueueDB.php +++ b/includes/jobqueue/JobQueueDB.php @@ -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 ); } /** diff --git a/includes/libs/rdbms/database/DBConnRef.php b/includes/libs/rdbms/database/DBConnRef.php index eb0e954e16..ef2953ec9c 100644 --- a/includes/libs/rdbms/database/DBConnRef.php +++ b/includes/libs/rdbms/database/DBConnRef.php @@ -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 ); diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php index c9403929a0..22a58055d7 100644 --- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php @@ -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 diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index 1df2409ae0..f64cd56f64 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -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 ); } diff --git a/includes/objectcache/SqlBagOStuff.php b/includes/objectcache/SqlBagOStuff.php index 3cce530c3c..2cfd2a1d76 100644 --- a/includes/objectcache/SqlBagOStuff.php +++ b/includes/objectcache/SqlBagOStuff.php @@ -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 index 0000000000..f8ab7f481b --- /dev/null +++ b/tests/phpunit/includes/db/LoadBalancerTest.php @@ -0,0 +1,135 @@ + $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(); + } +} -- 2.20.1