From: jenkins-bot Date: Sun, 25 Aug 2019 17:09:21 +0000 (+0000) Subject: Merge "Revert "rdbms: make LoadBalancer::reallyOpenConnection() handle setting DBO_TRX"" X-Git-Tag: 1.34.0-rc.0~588 X-Git-Url: http://git.cyclocoop.org/%7B%24www_url%7Dadmin/membres/fiche.php?a=commitdiff_plain;h=a34c284b1a7096b594288850bd175709b749aea3;hp=-c;p=lhc%2Fweb%2Fwiklou.git Merge "Revert "rdbms: make LoadBalancer::reallyOpenConnection() handle setting DBO_TRX"" --- a34c284b1a7096b594288850bd175709b749aea3 diff --combined includes/libs/rdbms/lbfactory/LBFactory.php index 9efb1ade67,4426654ca2..77467f062a --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@@ -155,12 -155,12 +155,11 @@@ abstract class LBFactory implements ILB $this->defaultGroup = $conf['defaultGroup'] ?? null; $this->secret = $conf['secret'] ?? ''; - static $nextId, $nextTicket; - $this->id = $nextId = ( is_int( $nextId ) ? $nextId++ : mt_rand() ); - $this->ticket = $nextTicket = ( is_int( $nextTicket ) ? $nextTicket++ : mt_rand() ); + $this->id = mt_rand(); + $this->ticket = mt_rand(); } public function destroy() { - $this->shutdown( self::SHUTDOWN_NO_CHRONPROT ); $this->forEachLBCallMethod( 'disable' ); } diff --combined includes/libs/rdbms/loadbalancer/LoadBalancer.php index d277f225cf,d088aa9e54..b890df6c0a --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@@ -101,8 -101,6 +101,8 @@@ class LoadBalancer implements ILoadBala private $indexAliases = []; /** @var array[] Map of (name => callable) */ private $trxRecurringCallbacks = []; + /** @var bool[] Map of (domain => whether to use "temp tables only" mode) */ + private $tempTablesOnlyMode = []; /** @var Database Connection handle that caused a problem */ private $errorConnection; @@@ -123,8 -121,6 +123,6 @@@ /** @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 */ @@@ -175,6 -171,11 +173,11 @@@ 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; @@@ -237,8 -238,6 +240,6 @@@ $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; } @@@ -298,10 -297,9 +299,10 @@@ /** * @param int $flags Bitfield of class CONN_* constants * @param int $i Specific server index or DB_MASTER/DB_REPLICA + * @param string $domain Database domain * @return int Sanitized bitfield */ - private function sanitizeConnectionFlags( $flags, $i ) { + private function sanitizeConnectionFlags( $flags, $i, $domain ) { // Whether an outside caller is explicitly requesting the master database server if ( $i === self::DB_MASTER || $i === $this->getWriterIndex() ) { $flags |= self::CONN_INTENT_WRITABLE; @@@ -322,12 -320,6 +323,12 @@@ $flags &= ~self::CONN_TRX_AUTOCOMMIT; $type = $this->getServerType( $this->getWriterIndex() ); $this->connLogger->info( __METHOD__ . ": CONN_TRX_AUTOCOMMIT disallowed ($type)" ); + } elseif ( isset( $this->tempTablesOnlyMode[$domain] ) ) { + // T202116: integration tests are active and queries should be all be using + // temporary clone tables (via prefix). Such tables are not visible accross + // different connections nor can there be REPEATABLE-READ snapshot staleness, + // so use the same connection for everything. + $flags &= ~self::CONN_TRX_AUTOCOMMIT; } } @@@ -882,7 -874,7 +883,7 @@@ public function getConnection( $i, $groups = [], $domain = false, $flags = 0 ) { $domain = $this->resolveDomainID( $domain ); $groups = $this->resolveGroups( $groups, $i ); - $flags = $this->sanitizeConnectionFlags( $flags, $i ); + $flags = $this->sanitizeConnectionFlags( $flags, $i, $domain ); // If given DB_MASTER/DB_REPLICA, resolve it to a specific server index. Resolving // DB_REPLICA might trigger getServerConnection() calls due to the getReaderIndex() // connectivity checks or LoadMonitor::scaleLoads() server state cache regeneration. @@@ -965,7 -957,17 +966,17 @@@ $serverIndex = $conn->getLBInfo( 'serverIndex' ); $refCount = $conn->getLBInfo( 'foreignPoolRefCount' ); if ( $serverIndex === null || $refCount === null ) { - return; // non-foreign connection; no domain-use tracking to update + /** + * 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; } 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. @@@ -1062,45 -1064,47 +1073,47 @@@ * * @note If disable() was called on this LoadBalancer, this method will throw a DBAccessError. * - * @param int $i Specific server index + * @param int $i 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 - $connKey = $autoCommit ? self::KEY_LOCAL_NOROUND : self::KEY_LOCAL; + $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 { - $conn = $this->reallyOpenConnection( - $i, - $this->localDomain, - [ 'autoCommitOnly' => $autoCommit ] - ); + // Open a new connection + $server = $this->getServerInfoStrict( $i ); + $server['serverIndex'] = $i; + $server['autoCommitOnly'] = $autoCommit; + $conn = $this->reallyOpenConnection( $server, $this->localDomain ); + $host = $this->getServerName( $i ); if ( $conn->isOpen() ) { - $this->connLogger->debug( __METHOD__ . ": opened new connection for $i" ); + $this->connLogger->debug( + __METHOD__ . ": connected to database $i at '$host'." ); $this->conns[$connKey][$i][0] = $conn; } else { - $this->connLogger->warning( __METHOD__ . ": connection error for $i" ); + $this->connLogger->warning( + __METHOD__ . ": failed to connect to database $i at '$host'." ); $this->errorConnection = $conn; $conn = false; } } - // Sanity check to make sure that the right domain is selected + // Final sanity check to make sure 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; @@@ -1122,7 -1126,7 +1135,7 @@@ * * @note If disable() was called on this LoadBalancer, this method will throw a DBAccessError. * - * @param int $i Specific server index + * @param int $i Server index * @param string $domain Domain ID to open * @param int $flags Class CONN_* constant bitfield * @return Database|bool Returns false on connection error @@@ -1132,9 -1136,10 +1145,10 @@@ */ 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; @@@ -1187,28 -1192,26 +1201,26 @@@ } if ( !$conn ) { - $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 { + // 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 { + // 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" ); } } if ( $conn instanceof IDatabase ) { - // Sanity check to make sure that the right domain is selected + // Final sanity check to make sure the right domain is selected if ( !$domainInstance->isCompatible( $conn->getDomainID() ) ) { throw new UnexpectedValueException( "Got connection to '{$conn->getDomainID()}', but expected '$domain'" ); @@@ -1243,101 -1246,94 +1255,94 @@@ * * Returns a Database object whether or not the connection was successful. * - * @param int $i Specific server index + * @param array $server * @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( $i, DatabaseDomain $domain, array $lbInfo ) { + protected function reallyOpenConnection( array $server, DatabaseDomain $domain ) { if ( $this->disabled ) { throw new DBAccessError(); } - $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 ); + 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 try { - $conn->initConnection(); + $db = Database::factory( $server['type'], $server ); + // Log when many connection are made on requests ++$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 ) { - // ignore; let the DB handle the logging + // 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; } - $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() ) { + $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() ) { if ( $this->trxRoundId !== false ) { - $this->applyTransactionRoundFlags( $conn ); + $this->applyTransactionRoundFlags( $db ); } foreach ( $this->trxRecurringCallbacks as $name => $callback ) { - $conn->setTransactionListener( $name, $callback ); + $db->setTransactionListener( $name, $callback ); } } - $conn->setTableAliases( $this->tableAliases ); - $conn->setIndexAliases( $this->indexAliases ); - - $conn->setLazyMasterHandle( - $this->getLazyConnectionRef( self::DB_MASTER, [], $conn->getDomainID() ) - ); $this->lazyLoadReplicationPositions(); // session consistency - // 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; + return $db; } /** @@@ -2113,8 -2109,8 +2118,8 @@@ public function waitForMasterPos( IDatabase $conn, $pos = false, $timeout = null ) { $timeout = max( 1, $timeout ?: $this->waitTimeout ); - if ( $conn->getLBInfo( 'serverIndex' ) === $this->getWriterIndex() ) { - return true; // not a replica DB server + if ( $this->getServerCount() <= 1 || !$conn->getLBInfo( 'replica' ) ) { + return true; // server is not a replica DB } if ( !$pos ) { @@@ -2230,9 -2226,9 +2235,9 @@@ ) ); // Update the prefix for all local connections... - $this->forEachOpenConnection( function ( IDatabase $conn ) use ( $prefix ) { - if ( !$conn->getLBInfo( 'foreign' ) ) { - $conn->tablePrefix( $prefix ); + $this->forEachOpenConnection( function ( IDatabase $db ) use ( $prefix ) { + if ( !$db->getLBInfo( 'foreign' ) ) { + $db->tablePrefix( $prefix ); } } ); } @@@ -2243,17 -2239,6 +2248,17 @@@ $this->setLocalDomain( DatabaseDomain::newFromId( $domain ) ); } + public function setTempTablesOnlyMode( $value, $domain ) { + $old = $this->tempTablesOnlyMode[$domain] ?? false; + if ( $value ) { + $this->tempTablesOnlyMode[$domain] = true; + } else { + unset( $this->tempTablesOnlyMode[$domain] ); + } + + return $old; + } + /** * @param DatabaseDomain $domain */