From 45831e619c5e667ae1201bcdacfb8d4dcce10b41 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sun, 14 Jul 2019 15:43:26 -0700 Subject: [PATCH] rdbms: make LoadBalancer::reallyOpenConnection() handle setting DBO_TRX Make LoadBalancer::reallyOpenConnection() handle initializing DBO_TRX instead of Database::__construct(). Also: * Avoid having the "catch" block appear like it returns a half-constructed Database. * Use the variable name $conn instead of $db to be consistent throughout the class. Only send Database::__construct() parameters that it recognizes instead of mixing in setLBInfo() data. Change-Id: Iffc3d1d0713051a164adb51a4c4ee12e4ac887c3 --- includes/libs/rdbms/database/Database.php | 10 +- .../libs/rdbms/database/DatabasePostgres.php | 6 +- .../libs/rdbms/database/DatabaseSqlite.php | 25 +- includes/libs/rdbms/database/IDatabase.php | 13 +- includes/libs/rdbms/lbfactory/LBFactory.php | 5 +- .../libs/rdbms/lbfactory/LBFactoryMulti.php | 2 - .../libs/rdbms/lbfactory/LBFactorySimple.php | 8 - .../libs/rdbms/loadbalancer/LoadBalancer.php | 231 +++++++++--------- .../rdbms/loadbalancer/LoadBalancerSingle.php | 2 +- .../Revision/RevisionStoreDbTestBase.php | 8 +- .../includes/db/DatabasePostgresTest.php | 4 +- tests/phpunit/includes/db/LBFactoryTest.php | 23 +- .../rdbms/database/DatabaseMysqlBaseTest.php | 2 + 13 files changed, 166 insertions(+), 173 deletions(-) diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 03180228a3..f1b2ce277f 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -174,6 +174,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware /** @var float Query rount trip time estimate */ private $lastRoundTripEstimate = 0.0; + /** @var string Whether the database is a file on disk */ + const ATTR_DB_IS_FILE = 'db-is-file'; /** @var string Lock granularity is on the level of the entire database */ const ATTR_DB_LEVEL_LOCKING = 'db-level-locking'; /** @var string The SCHEMA keyword refers to a grouping of tables in a database */ @@ -245,13 +247,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->cliMode = $params['cliMode']; $this->agent = $params['agent']; $this->flags = $params['flags']; - if ( $this->flags & self::DBO_DEFAULT ) { - if ( $this->cliMode ) { - $this->flags &= ~self::DBO_TRX; - } else { - $this->flags |= self::DBO_TRX; - } - } $this->nonNativeInsertSelectBatchSize = $params['nonNativeInsertSelectBatchSize'] ?? 10000; $this->srvCache = $params['srvCache'] ?? new HashBagOStuff(); @@ -425,6 +420,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware */ final public static function attributesFromType( $dbType, $driver = null ) { static $defaults = [ + self::ATTR_DB_IS_FILE => false, self::ATTR_DB_LEVEL_LOCKING => false, self::ATTR_SCHEMAS_AS_TABLE_GROUPS => false ]; diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index a7ebc86e50..197cdcc196 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -95,8 +95,8 @@ class DatabasePostgres extends Database { $this->password = $password; $connectVars = [ - // pg_connect() user $user as the default database. Since a database is required, - // then pick a "don't care" database that is more likely to exist than that one. + // A database must be specified in order to connect to Postgres. If $dbName is not + // specified, then use the standard "postgres" database that should exist by default. 'dbname' => strlen( $dbName ) ? $dbName : 'postgres', 'user' => $user, 'password' => $password @@ -1442,7 +1442,7 @@ SQL; return $row ? ( strtolower( $row->default_transaction_read_only ) === 'on' ) : false; } - public static function getAttributes() { + protected static function getAttributes() { return [ self::ATTR_SCHEMAS_AS_TABLE_GROUPS => true ]; } diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php index b1521dca2f..8d417e659f 100644 --- a/includes/libs/rdbms/database/DatabaseSqlite.php +++ b/includes/libs/rdbms/database/DatabaseSqlite.php @@ -68,21 +68,21 @@ class DatabaseSqlite extends Database { * - dbDirectory : directory containing the DB and the lock file directory * - dbFilePath : use this to force the path of the DB file * - trxMode : one of (deferred, immediate, exclusive) - * @param array $p + * @param array $params */ - public function __construct( array $p ) { - if ( isset( $p['dbFilePath'] ) ) { - $this->dbPath = $p['dbFilePath']; - if ( !strlen( $p['dbname'] ) ) { - $p['dbname'] = self::generateDatabaseName( $this->dbPath ); + public function __construct( array $params ) { + if ( isset( $params['dbFilePath'] ) ) { + $this->dbPath = $params['dbFilePath']; + if ( !strlen( $params['dbname'] ) ) { + $params['dbname'] = self::generateDatabaseName( $this->dbPath ); } - } elseif ( isset( $p['dbDirectory'] ) ) { - $this->dbDir = $p['dbDirectory']; + } elseif ( isset( $params['dbDirectory'] ) ) { + $this->dbDir = $params['dbDirectory']; } - parent::__construct( $p ); + parent::__construct( $params ); - $this->trxMode = strtoupper( $p['trxMode'] ?? '' ); + $this->trxMode = strtoupper( $params['trxMode'] ?? '' ); $lockDirectory = $this->getLockFileDirectory(); if ( $lockDirectory !== null ) { @@ -96,7 +96,10 @@ class DatabaseSqlite extends Database { } protected static function getAttributes() { - return [ self::ATTR_DB_LEVEL_LOCKING => true ]; + return [ + self::ATTR_DB_IS_FILE => true, + self::ATTR_DB_LEVEL_LOCKING => true + ]; } /** diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php index 876821319e..a64078abd4 100644 --- a/includes/libs/rdbms/database/IDatabase.php +++ b/includes/libs/rdbms/database/IDatabase.php @@ -1082,8 +1082,9 @@ interface IDatabase { * * In systems like mysql/mariadb, different databases can easily be referenced on a single * connection merely by name, even in a single query via JOIN. On the other hand, Postgres - * treats databases as fully separate, only allowing mechanisms like postgres_fdw to - * effectively "mount" foreign DBs. This is true even among DBs on the same server. + * treats databases as logically separate, with different database users, requiring special + * mechanisms like postgres_fdw to "mount" foreign DBs. This is true even among DBs on the + * same server. Changing the selected database via selectDomain() requires a new connection. * * @return bool * @since 1.29 @@ -2032,11 +2033,11 @@ interface IDatabase { public function setSessionOptions( array $options ); /** - * Set variables to be used in sourceFile/sourceStream, in preference to the - * ones in $GLOBALS. If an array is set here, $GLOBALS will not be used at - * all. If it's set to false, $GLOBALS will be used. + * Set schema variables to be used when streaming commands from SQL files or stdin * - * @param bool|array $vars Mapping variable name to value. + * Variables appear as SQL comments and are substituted by their corresponding values + * + * @param array|null $vars Map of (variable => value) or null to use the defaults */ public function setSchemaVars( $vars ); diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index 4426654ca2..c2e4c38cae 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -155,8 +155,9 @@ abstract class LBFactory implements ILBFactory { $this->defaultGroup = $conf['defaultGroup'] ?? null; $this->secret = $conf['secret'] ?? ''; - $this->id = mt_rand(); - $this->ticket = mt_rand(); + static $nextId, $nextTicket; + $this->id = $nextId = ( is_int( $nextId ) ? $nextId++ : mt_rand() ); + $this->ticket = $nextTicket = ( is_int( $nextTicket ) ? $nextTicket++ : mt_rand() ); } public function destroy() { diff --git a/includes/libs/rdbms/lbfactory/LBFactoryMulti.php b/includes/libs/rdbms/lbfactory/LBFactoryMulti.php index f675b58778..1a3495e4ac 100644 --- a/includes/libs/rdbms/lbfactory/LBFactoryMulti.php +++ b/includes/libs/rdbms/lbfactory/LBFactoryMulti.php @@ -312,13 +312,11 @@ class LBFactoryMulti extends LBFactory { foreach ( $loads as $serverName => $load ) { $serverInfo = $template; if ( $master ) { - $serverInfo['master'] = true; if ( $this->masterTemplateOverrides ) { $serverInfo = $this->masterTemplateOverrides + $serverInfo; } $master = false; } else { - $serverInfo['replica'] = true; } if ( isset( $this->templateOverridesByServer[$serverName] ) ) { $serverInfo = $this->templateOverridesByServer[$serverName] + $serverInfo; diff --git a/includes/libs/rdbms/lbfactory/LBFactorySimple.php b/includes/libs/rdbms/lbfactory/LBFactorySimple.php index fd76d88dd0..eb4780288e 100644 --- a/includes/libs/rdbms/lbfactory/LBFactorySimple.php +++ b/includes/libs/rdbms/lbfactory/LBFactorySimple.php @@ -58,14 +58,6 @@ class LBFactorySimple extends LBFactory { parent::__construct( $conf ); $this->servers = $conf['servers'] ?? []; - foreach ( $this->servers as $i => $server ) { - if ( $i == 0 ) { - $this->servers[$i]['master'] = true; - } else { - $this->servers[$i]['replica'] = true; - } - } - $this->externalClusters = $conf['externalClusters'] ?? []; $this->loadMonitorClass = $conf['loadMonitorClass'] ?? 'LoadMonitor'; } diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index d088aa9e54..955e3d83af 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -121,6 +121,8 @@ class LoadBalancer implements ILoadBalancer { /** @var bool Whether any connection has been attempted yet */ private $connectionAttempted = false; + /** var int An identifier for this class instance */ + private $id; /** @var int|null Integer ID of the managing LBFactory instance or null if none */ private $ownerId; /** @var string|bool Explicit DBO_TRX transaction round active or false if none */ @@ -171,11 +173,6 @@ class LoadBalancer implements ILoadBalancer { if ( ++$listKey !== $i ) { throw new UnexpectedValueException( 'List expected for "servers" parameter' ); } - if ( $i == 0 ) { - $server['master'] = true; - } else { - $server['replica'] = true; - } $this->servers[$i] = $server; foreach ( ( $server['groupLoads'] ?? [] ) as $group => $ratio ) { $this->groupLoads[$group][$i] = $ratio; @@ -238,6 +235,8 @@ class LoadBalancer implements ILoadBalancer { $group = $params['defaultGroup'] ?? self::GROUP_GENERIC; $this->defaultGroup = isset( $this->groupLoads[$group] ) ? $group : self::GROUP_GENERIC; + static $nextId; + $this->id = $nextId = ( is_int( $nextId ) ? $nextId++ : mt_rand() ); $this->ownerId = $params['ownerId'] ?? null; } @@ -957,17 +956,7 @@ class LoadBalancer implements ILoadBalancer { $serverIndex = $conn->getLBInfo( 'serverIndex' ); $refCount = $conn->getLBInfo( 'foreignPoolRefCount' ); if ( $serverIndex === null || $refCount === null ) { - /** - * This can happen in code like: - * foreach ( $dbs as $db ) { - * $conn = $lb->getConnection( $lb::DB_REPLICA, [], $db ); - * ... - * $lb->reuseConnection( $conn ); - * } - * When a connection to the local DB is opened in this way, reuseConnection() - * should be ignored - */ - return; + return; // non-foreign connection; no domain-use tracking to update } elseif ( $conn instanceof DBConnRef ) { // DBConnRef already handles calling reuseConnection() and only passes the live // Database instance to this method. Any caller passing in a DBConnRef is broken. @@ -1064,47 +1053,45 @@ class LoadBalancer implements ILoadBalancer { * * @note If disable() was called on this LoadBalancer, this method will throw a DBAccessError. * - * @param int $i Server index + * @param int $i Specific server index * @param int $flags Class CONN_* constant bitfield * @return Database * @throws InvalidArgumentException When the server index is invalid * @throws UnexpectedValueException When the DB domain of the connection is corrupted */ private function getLocalConnection( $i, $flags = 0 ) { + $autoCommit = ( ( $flags & self::CONN_TRX_AUTOCOMMIT ) == self::CONN_TRX_AUTOCOMMIT ); // Connection handles required to be in auto-commit mode use a separate connection // pool since the main pool is effected by implicit and explicit transaction rounds - $autoCommit = ( ( $flags & self::CONN_TRX_AUTOCOMMIT ) == self::CONN_TRX_AUTOCOMMIT ); - $connKey = $autoCommit ? self::KEY_LOCAL_NOROUND : self::KEY_LOCAL; + if ( isset( $this->conns[$connKey][$i][0] ) ) { $conn = $this->conns[$connKey][$i][0]; } else { - // Open a new connection - $server = $this->getServerInfoStrict( $i ); - $server['serverIndex'] = $i; - $server['autoCommitOnly'] = $autoCommit; - $conn = $this->reallyOpenConnection( $server, $this->localDomain ); - $host = $this->getServerName( $i ); + $conn = $this->reallyOpenConnection( + $i, + $this->localDomain, + [ 'autoCommitOnly' => $autoCommit ] + ); if ( $conn->isOpen() ) { - $this->connLogger->debug( - __METHOD__ . ": connected to database $i at '$host'." ); + $this->connLogger->debug( __METHOD__ . ": opened new connection for $i" ); $this->conns[$connKey][$i][0] = $conn; } else { - $this->connLogger->warning( - __METHOD__ . ": failed to connect to database $i at '$host'." ); + $this->connLogger->warning( __METHOD__ . ": connection error for $i" ); $this->errorConnection = $conn; $conn = false; } } - // Final sanity check to make sure the right domain is selected + // Sanity check to make sure that the right domain is selected if ( $conn instanceof IDatabase && !$this->localDomain->isCompatible( $conn->getDomainID() ) ) { throw new UnexpectedValueException( "Got connection to '{$conn->getDomainID()}', " . - "but expected local domain ('{$this->localDomain}')" ); + "but expected local domain ('{$this->localDomain}')" + ); } return $conn; @@ -1126,7 +1113,7 @@ class LoadBalancer implements ILoadBalancer { * * @note If disable() was called on this LoadBalancer, this method will throw a DBAccessError. * - * @param int $i Server index + * @param int $i Specific server index * @param string $domain Domain ID to open * @param int $flags Class CONN_* constant bitfield * @return Database|bool Returns false on connection error @@ -1136,10 +1123,9 @@ class LoadBalancer implements ILoadBalancer { */ private function getForeignConnection( $i, $domain, $flags = 0 ) { $domainInstance = DatabaseDomain::newFromId( $domain ); + $autoCommit = ( ( $flags & self::CONN_TRX_AUTOCOMMIT ) == self::CONN_TRX_AUTOCOMMIT ); // Connection handles required to be in auto-commit mode use a separate connection // pool since the main pool is effected by implicit and explicit transaction rounds - $autoCommit = ( ( $flags & self::CONN_TRX_AUTOCOMMIT ) == self::CONN_TRX_AUTOCOMMIT ); - if ( $autoCommit ) { $connFreeKey = self::KEY_FOREIGN_FREE_NOROUND; $connInUseKey = self::KEY_FOREIGN_INUSE_NOROUND; @@ -1192,26 +1178,28 @@ class LoadBalancer implements ILoadBalancer { } if ( !$conn ) { - // Open a new connection - $server = $this->getServerInfoStrict( $i ); - $server['serverIndex'] = $i; - $server['foreignPoolRefCount'] = 0; - $server['foreign'] = true; - $server['autoCommitOnly'] = $autoCommit; - $conn = $this->reallyOpenConnection( $server, $domainInstance ); - if ( !$conn->isOpen() ) { - $this->connLogger->warning( __METHOD__ . ": connection error for $i/$domain" ); - $this->errorConnection = $conn; - $conn = false; - } else { + $conn = $this->reallyOpenConnection( + $i, + $domainInstance, + [ + 'autoCommitOnly' => $autoCommit, + 'foreign' => true, + 'foreignPoolRefCount' => 0 + ] + ); + if ( $conn->isOpen() ) { // Note that if $domain is an empty string, getDomainID() might not match it $this->conns[$connInUseKey][$i][$conn->getDomainID()] = $conn; $this->connLogger->debug( __METHOD__ . ": opened new connection for $i/$domain" ); + } else { + $this->connLogger->warning( __METHOD__ . ": connection error for $i/$domain" ); + $this->errorConnection = $conn; + $conn = false; } } if ( $conn instanceof IDatabase ) { - // Final sanity check to make sure the right domain is selected + // Sanity check to make sure that the right domain is selected if ( !$domainInstance->isCompatible( $conn->getDomainID() ) ) { throw new UnexpectedValueException( "Got connection to '{$conn->getDomainID()}', but expected '$domain'" ); @@ -1246,94 +1234,101 @@ class LoadBalancer implements ILoadBalancer { * * Returns a Database object whether or not the connection was successful. * - * @param array $server + * @param int $i Specific server index * @param DatabaseDomain $domain Domain the connection is for, possibly unspecified + * @param array $lbInfo Additional information for setLBInfo() * @return Database * @throws DBAccessError * @throws InvalidArgumentException */ - protected function reallyOpenConnection( array $server, DatabaseDomain $domain ) { + protected function reallyOpenConnection( $i, DatabaseDomain $domain, array $lbInfo ) { if ( $this->disabled ) { throw new DBAccessError(); } - if ( $domain->getDatabase() === null ) { - // The database domain does not specify a DB name and some database systems require a - // valid DB specified on connection. The $server configuration array contains a default - // DB name to use for connections in such cases. - if ( $server['type'] === 'mysql' ) { - // For MySQL, DATABASE and SCHEMA are synonyms, connections need not specify a DB, - // and the DB name in $server might not exist due to legacy reasons (the default - // domain used to ignore the local LB domain, even when mismatched). - $server['dbname'] = null; - } - } else { - $server['dbname'] = $domain->getDatabase(); - } - - if ( $domain->getSchema() !== null ) { - $server['schema'] = $domain->getSchema(); - } - - // It is always possible to connect with any prefix, even the empty string - $server['tablePrefix'] = $domain->getTablePrefix(); - - // Let the handle know what the cluster master is (e.g. "db1052") - $masterName = $this->getServerName( $this->getWriterIndex() ); - $server['clusterMasterHost'] = $masterName; - - $server['srvCache'] = $this->srvCache; - // Set loggers and profilers - $server['connLogger'] = $this->connLogger; - $server['queryLogger'] = $this->queryLogger; - $server['errorLogger'] = $this->errorLogger; - $server['deprecationLogger'] = $this->deprecationLogger; - $server['profiler'] = $this->profiler; - $server['trxProfiler'] = $this->trxProfiler; - // Use the same agent and PHP mode for all DB handles - $server['cliMode'] = $this->cliMode; - $server['agent'] = $this->agent; - // Use DBO_DEFAULT flags by default for LoadBalancer managed databases. Assume that the - // application calls LoadBalancer::commitMasterChanges() before the PHP script completes. - $server['flags'] = $server['flags'] ?? IDatabase::DBO_DEFAULT; - - // Create a live connection object + $server = $this->getServerInfoStrict( $i ); + $server = array_merge( $server, [ + // Use the database specified in $domain (null means "none or entrypoint DB"); + // fallback to the $server default if the RDBMs is an embedded library using a file + // on disk since there would be nothing to access to without a DB/file name. + 'dbname' => $this->getServerAttributes( $i )[Database::ATTR_DB_IS_FILE] + ? ( $domain->getDatabase() ?? $server['dbname'] ?? null ) + : $domain->getDatabase(), + // Override the $server default schema with that of $domain if specified + 'schema' => $domain->getSchema() ?? $server['schema'] ?? null, + // Use the table prefix specified in $domain + 'tablePrefix' => $domain->getTablePrefix(), + // Participate in transaction rounds if $server does not specify otherwise + 'flags' => $server['flags'] ?? IDatabase::DBO_DEFAULT, + // Inject the PHP execution mode and the agent string + 'cliMode' => $this->cliMode, + 'agent' => $this->agent, + // Inject object and callback dependencies + 'srvCache' => $this->srvCache, + 'connLogger' => $this->connLogger, + 'queryLogger' => $this->queryLogger, + 'errorLogger' => $this->errorLogger, + 'deprecationLogger' => $this->deprecationLogger, + 'profiler' => $this->profiler, + 'trxProfiler' => $this->trxProfiler + ] ); + + $lbInfo = array_merge( $lbInfo, [ + 'ownerId' => $this->id, + 'serverIndex' => $i, + 'master' => ( $i === $this->getWriterIndex() ), + 'replica' => ( $i !== $this->getWriterIndex() ), + // Name of the master server of the relevant DB cluster (e.g. "db1052") + 'clusterMasterHost' => $this->getServerName( $this->getWriterIndex() ) + ] ); + + $conn = Database::factory( $server['type'], $server, Database::NEW_UNCONNECTED ); try { - $db = Database::factory( $server['type'], $server ); - // Log when many connection are made on requests + $conn->initConnection(); ++$this->connectionCounter; - $currentConnCount = $this->getCurrentConnectionCount(); - if ( $currentConnCount >= self::CONN_HELD_WARN_THRESHOLD ) { - $this->perfLogger->warning( - __METHOD__ . ": {connections}+ connections made (master={masterdb})", - [ 'connections' => $currentConnCount, 'masterdb' => $masterName ] - ); - } } catch ( DBConnectionError $e ) { - // FIXME: This is probably the ugliest thing I have ever done to - // PHP. I'm half-expecting it to segfault, just out of disgust. -- TS - $db = $e->db; + // ignore; let the DB handle the logging } - $db->setLBInfo( $server ); - $db->setLazyMasterHandle( - $this->getLazyConnectionRef( self::DB_MASTER, [], $db->getDomainID() ) - ); - $db->setTableAliases( $this->tableAliases ); - $db->setIndexAliases( $this->indexAliases ); - - if ( $server['serverIndex'] === $this->getWriterIndex() ) { + $conn->setLBInfo( $lbInfo ); + if ( $conn->getFlag( $conn::DBO_DEFAULT ) ) { + if ( $this->cliMode ) { + $conn->clearFlag( $conn::DBO_TRX ); + } else { + $conn->setFlag( $conn::DBO_TRX ); + } + } + if ( $i === $this->getWriterIndex() ) { if ( $this->trxRoundId !== false ) { - $this->applyTransactionRoundFlags( $db ); + $this->applyTransactionRoundFlags( $conn ); } foreach ( $this->trxRecurringCallbacks as $name => $callback ) { - $db->setTransactionListener( $name, $callback ); + $conn->setTransactionListener( $name, $callback ); } } + $conn->setTableAliases( $this->tableAliases ); + $conn->setIndexAliases( $this->indexAliases ); + + $conn->setLazyMasterHandle( + $this->getLazyConnectionRef( self::DB_MASTER, [], $conn->getDomainID() ) + ); $this->lazyLoadReplicationPositions(); // session consistency - return $db; + // Log when many connection are made on requests + $count = $this->getCurrentConnectionCount(); + if ( $count >= self::CONN_HELD_WARN_THRESHOLD ) { + $this->perfLogger->warning( + __METHOD__ . ": {connections}+ connections made (master={masterdb})", + [ + 'connections' => $count, + 'dbserver' => $conn->getServer(), + 'masterdb' => $conn->getLBInfo( 'clusterMasterHost' ) + ] + ); + } + + return $conn; } /** @@ -2109,8 +2104,8 @@ class LoadBalancer implements ILoadBalancer { public function waitForMasterPos( IDatabase $conn, $pos = false, $timeout = null ) { $timeout = max( 1, $timeout ?: $this->waitTimeout ); - if ( $this->getServerCount() <= 1 || !$conn->getLBInfo( 'replica' ) ) { - return true; // server is not a replica DB + if ( $conn->getLBInfo( 'serverIndex' ) === $this->getWriterIndex() ) { + return true; // not a replica DB server } if ( !$pos ) { @@ -2226,9 +2221,9 @@ class LoadBalancer implements ILoadBalancer { ) ); // Update the prefix for all local connections... - $this->forEachOpenConnection( function ( IDatabase $db ) use ( $prefix ) { - if ( !$db->getLBInfo( 'foreign' ) ) { - $db->tablePrefix( $prefix ); + $this->forEachOpenConnection( function ( IDatabase $conn ) use ( $prefix ) { + if ( !$conn->getLBInfo( 'foreign' ) ) { + $conn->tablePrefix( $prefix ); } } ); } diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php b/includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php index 4c68833e7e..2ad24b95d0 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php @@ -83,7 +83,7 @@ class LoadBalancerSingle extends LoadBalancer { ) ); } - protected function reallyOpenConnection( array $server, DatabaseDomain $domain ) { + protected function reallyOpenConnection( $i, DatabaseDomain $domain, array $lbInfo = [] ) { return $this->db; } diff --git a/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php b/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php index d4393dd3f6..ba2d28ea0d 100644 --- a/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php +++ b/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php @@ -23,6 +23,7 @@ use MediaWiki\Storage\SqlBlobStore; use MediaWiki\User\UserIdentityValue; use MediaWikiTestCase; use PHPUnit_Framework_MockObject_MockObject; +use Psr\Log\NullLogger; use Revision; use TestUserRegistry; use Title; @@ -146,7 +147,7 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase { ->getMock(); $lb->method( 'reallyOpenConnection' )->willReturnCallback( - function ( array $server, $dbNameOverride ) { + function () use ( $server ) { return $this->getDatabaseMock( $server ); } ); @@ -207,10 +208,11 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase { 'cliMode' => true, 'agent' => '', 'load' => 100, + 'srvCache' => new HashBagOStuff(), 'profiler' => null, 'trxProfiler' => new TransactionProfiler(), - 'connLogger' => new \Psr\Log\NullLogger(), - 'queryLogger' => new \Psr\Log\NullLogger(), + 'connLogger' => new NullLogger(), + 'queryLogger' => new NullLogger(), 'errorLogger' => function () { }, 'deprecationLogger' => function () { diff --git a/tests/phpunit/includes/db/DatabasePostgresTest.php b/tests/phpunit/includes/db/DatabasePostgresTest.php index e0b81117f9..31a634370a 100644 --- a/tests/phpunit/includes/db/DatabasePostgresTest.php +++ b/tests/phpunit/includes/db/DatabasePostgresTest.php @@ -184,6 +184,8 @@ class DatabasePostgresTest extends MediaWikiTestCase { * @covers \Wikimedia\Rdbms\DatabasePostgres::getAttributes */ public function testAttributes() { - $this->assertTrue( DatabasePostgres::getAttributes()[Database::ATTR_SCHEMAS_AS_TABLE_GROUPS] ); + $this->assertTrue( + Database::attributesFromType( 'postgres' )[Database::ATTR_SCHEMAS_AS_TABLE_GROUPS] + ); } } diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php index 1016f28a38..1296bb64b5 100644 --- a/tests/phpunit/includes/db/LBFactoryTest.php +++ b/tests/phpunit/includes/db/LBFactoryTest.php @@ -23,6 +23,7 @@ * @copyright © 2013 Wikimedia Foundation Inc. */ +use Wikimedia\AtEase\AtEase; use Wikimedia\Rdbms\IDatabase; use Wikimedia\Rdbms\IMaintainableDatabase; use Wikimedia\Rdbms\LBFactory; @@ -474,7 +475,7 @@ class LBFactoryTest extends MediaWikiTestCase { unset( $db ); /** @var IMaintainableDatabase $db */ - $db = $lb->getConnection( DB_MASTER, [], '' ); + $db = $lb->getConnection( DB_MASTER, [], $lb::DOMAIN_ANY ); $this->assertEquals( '', @@ -554,7 +555,7 @@ class LBFactoryTest extends MediaWikiTestCase { ); $lb = $factory->getMainLB(); /** @var IMaintainableDatabase $db */ - $db = $lb->getConnection( DB_MASTER, [], '' ); + $db = $lb->getConnection( DB_MASTER, [], $lb::DOMAIN_ANY ); $this->assertEquals( '', $db->getDomainID(), "Null domain used" ); @@ -622,16 +623,16 @@ class LBFactoryTest extends MediaWikiTestCase { ); $lb = $factory->getMainLB(); /** @var IDatabase $db */ - $db = $lb->getConnection( DB_MASTER, [], '' ); + $db = $lb->getConnection( DB_MASTER, [], $lb::DOMAIN_ANY ); - \Wikimedia\suppressWarnings(); + AtEase::suppressWarnings(); try { - $this->assertFalse( $db->selectDB( 'garbage-db' ) ); + $this->assertFalse( $db->selectDomain( 'garbagedb' ) ); $this->fail( "No error thrown." ); } catch ( \Wikimedia\Rdbms\DBQueryError $e ) { - $this->assertRegExp( '/[\'"]garbage-db[\'"]/', $e->getMessage() ); + $this->assertRegExp( '/[\'"]garbagedb[\'"]/', $e->getMessage() ); } - \Wikimedia\restoreWarnings(); + AtEase::restoreWarnings(); } /** @@ -650,12 +651,12 @@ class LBFactoryTest extends MediaWikiTestCase { ); $lb = $factory->getMainLB(); - if ( !$lb->getConnection( DB_MASTER )->databasesAreIndependent() ) { - $this->markTestSkipped( "Not applicable per databasesAreIndependent()" ); + if ( !$factory->getMainLB()->getServerAttributes( 0 )[Database::ATTR_DB_IS_FILE] ) { + $this->markTestSkipped( "Not applicable per ATTR_DB_IS_FILE" ); } /** @var IDatabase $db */ - $lb->getConnection( DB_MASTER, [], '' ); + $this->assertNotNull( $lb->getConnection( DB_MASTER, [], $lb::DOMAIN_ANY ) ); } /** @@ -679,7 +680,7 @@ class LBFactoryTest extends MediaWikiTestCase { } $db = $lb->getConnection( DB_MASTER ); - $db->selectDB( 'garbage-db' ); + $db->selectDomain( 'garbage-db' ); } /** diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseMysqlBaseTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseMysqlBaseTest.php index 4c92545128..ee2d174f75 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseMysqlBaseTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseMysqlBaseTest.php @@ -362,6 +362,8 @@ class DatabaseMysqlBaseTest extends PHPUnit\Framework\TestCase { $db->method( 'getMasterServerInfo' ) ->willReturn( [ 'serverId' => 172, 'asOf' => time() ] ); + $db->setLBInfo( 'replica', true ); + // Fake the current time. list( $nowSecFrac, $nowSec ) = explode( ' ', microtime() ); $now = (float)$nowSec + (float)$nowSecFrac; -- 2.20.1