From 3d5faa10f4028f9742df5aaed86a1889b928dc26 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Mon, 26 Aug 2019 11:17:12 -0700 Subject: [PATCH] rdbms: clean up use of ATTACH queries in DatabaseSqlite Defer the queries until a connection exists. Only issue issue the them for databases that are different than the currently opened file. Also, make handleSessionLossPreconnect() aware of attached databases. In LoadBalancer::reallyOpenConnection(), avoid having the "catch" block appear like it returns a half-constructed Database. Change-Id: I9f676bb72a1ab06f0eac5820dce28231741c283d --- includes/libs/rdbms/database/Database.php | 9 +++++ .../libs/rdbms/database/DatabaseSqlite.php | 26 ++++++++++--- .../libs/rdbms/loadbalancer/LoadBalancer.php | 38 ++++++++++--------- 3 files changed, 51 insertions(+), 22 deletions(-) diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index b5ec6521a5..6aa3f6fe59 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -1487,6 +1487,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->trxAtomicCounter = 0; $this->trxIdleCallbacks = []; // T67263; transaction already lost $this->trxPreCommitCallbacks = []; // T67263; transaction already lost + // Clear additional subclass fields + $this->doHandleSessionLossPreconnect(); // @note: leave trxRecurringCallbacks in place if ( $this->trxDoneWrites ) { $this->trxProfiler->transactionWritingOut( @@ -1499,6 +1501,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } } + /** + * Reset any additional subclass trx* and session* fields + */ + protected function doHandleSessionLossPreconnect() { + // no-op + } + /** * Clean things up after session (and thus transaction) loss after reconnect */ diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php index 2977291ca6..657d308310 100644 --- a/includes/libs/rdbms/database/DatabaseSqlite.php +++ b/includes/libs/rdbms/database/DatabaseSqlite.php @@ -55,7 +55,7 @@ class DatabaseSqlite extends Database { protected $lockMgr; /** @var array List of shared database already attached to this connection */ - private $alreadyAttached = []; + private $sessionAttachedDbs = []; /** @var string[] See https://www.sqlite.org/lang_transaction.html */ private static $VALID_TRX_MODES = [ '', 'DEFERRED', 'IMMEDIATE', 'EXCLUSIVE' ]; @@ -182,6 +182,7 @@ class DatabaseSqlite extends Database { if ( in_array( $sync, [ 'EXTRA', 'FULL', 'NORMAL', 'OFF' ], true ) ) { $this->query( "PRAGMA synchronous = $sync", __METHOD__, $flags ); } + $this->attachDatabasesFromTableAliases(); } catch ( Exception $e ) { throw $this->newExceptionAfterConnectError( $e->getMessage() ); } @@ -1124,12 +1125,23 @@ class DatabaseSqlite extends Database { public function setTableAliases( array $aliases ) { parent::setTableAliases( $aliases ); + if ( $this->isOpen() ) { + $this->attachDatabasesFromTableAliases(); + } + } + + /** + * Issue ATTATCH statements for all unattached foreign DBs in table aliases + */ + private function attachDatabasesFromTableAliases() { foreach ( $this->tableAliases as $params ) { - if ( isset( $this->alreadyAttached[$params['dbname']] ) ) { - continue; + if ( + $params['dbname'] !== $this->getDBname() && + !isset( $this->sessionAttachedDbs[$params['dbname']] ) + ) { + $this->attachDatabase( $params['dbname'] ); + $this->sessionAttachedDbs[$params['dbname']] = true; } - $this->attachDatabase( $params['dbname'] ); - $this->alreadyAttached[$params['dbname']] = true; } } @@ -1147,6 +1159,10 @@ class DatabaseSqlite extends Database { return true; } + protected function doHandleSessionLossPreconnect() { + $this->sessionAttachedDbs = []; + } + /** * @return PDO */ diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index 771700c5a1..f60e8db1be 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -1303,23 +1303,7 @@ class LoadBalancer implements ILoadBalancer { $server['flags'] = $server['flags'] ?? IDatabase::DBO_DEFAULT; // Create a live connection object - try { - $conn = 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 ) { - // 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 - $conn = $e->db; - } - + $conn = Database::factory( $server['type'], $server, Database::NEW_UNCONNECTED ); $conn->setLBInfo( $server ); $conn->setLazyMasterHandle( $this->getLazyConnectionRef( self::DB_MASTER, [], $conn->getDomainID() ) @@ -1327,6 +1311,13 @@ class LoadBalancer implements ILoadBalancer { $conn->setTableAliases( $this->tableAliases ); $conn->setIndexAliases( $this->indexAliases ); + try { + $conn->initConnection(); + ++$this->connectionCounter; + } catch ( DBConnectionError $e ) { + // ignore; let the DB handle the logging + } + if ( $server['serverIndex'] === $this->getWriterIndex() ) { if ( $this->trxRoundId !== false ) { $this->applyTransactionRoundFlags( $conn ); @@ -1338,6 +1329,19 @@ class LoadBalancer implements ILoadBalancer { $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; } -- 2.20.1