From 14ee3f2107826495b8ef0d50e0340394bf28c92d Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 12 Jan 2018 13:44:12 -0800 Subject: [PATCH] rdbms: specify DB name and table prefix even for the local domain When LoadBalancer opens new local domain connections, it currently assumes that the domain specified by the server info array is the same. For sanity, make sure that the handle is set to the local domain. The main LBFactory/LoadBalancer use $wgDBname/$wgDBprefix as the local domain, corresponding with wfWikiId(). This relation is set automatically in MWLBFactory. If $wgLBFactoryConf/$wgDBservers is manually configured in a way breaking this correspondance, then it is misconfigured. Fixes made to avoid test failure: * Make sure LoadBalancer::setDomainPrefix() updates the local domain alias member. Also do not bother changing the domain of foreign connections. * Use the right domain ID for the connection array key names in LoadBalancer::openForeignConnection(). * Now that JobQueueTest no longer mistakenly uses the non-test tables, force it to use the main DB_MASTER handle so that it can see the unit test tables even if they are TEMPORARY; such tables are tied to the TCP connection, so separate handles see different temporary tables. Change-Id: I56f8b32fe957f984b8c9753e6db3b20abe96b038 --- .../libs/rdbms/loadbalancer/LoadBalancer.php | 63 ++++++++++++------- .../rdbms/loadbalancer/LoadBalancerSingle.php | 2 +- .../includes/Storage/RevisionStoreDbTest.php | 2 +- tests/phpunit/includes/db/LBFactoryTest.php | 49 ++++++++++----- .../includes/jobqueue/JobQueueTest.php | 12 +++- 5 files changed, 85 insertions(+), 43 deletions(-) diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index e80b9520e5..b01b23f4dd 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -146,17 +146,10 @@ class LoadBalancer implements ILoadBalancer { } } - $this->localDomain = isset( $params['localDomain'] ) + $localDomain = isset( $params['localDomain'] ) ? DatabaseDomain::newFromId( $params['localDomain'] ) : DatabaseDomain::newUnspecified(); - // In case a caller assumes that the domain ID is simply -, which is almost - // always true, gracefully handle the case when they fail to account for escaping. - if ( $this->localDomain->getTablePrefix() != '' ) { - $this->localDomainIdAlias = - $this->localDomain->getDatabase() . '-' . $this->localDomain->getTablePrefix(); - } else { - $this->localDomainIdAlias = $this->localDomain->getDatabase(); - } + $this->setLocalDomain( $localDomain ); $this->mWaitTimeout = isset( $params['waitTimeout'] ) ? $params['waitTimeout'] : 10; @@ -821,7 +814,11 @@ class LoadBalancer implements ILoadBalancer { $server = $this->mServers[$i]; $server['serverIndex'] = $i; $server['autoCommitOnly'] = $autoCommit; - $conn = $this->reallyOpenConnection( $server, false ); + if ( $this->localDomain->getDatabase() !== null ) { + // Use the local domain table prefix if the local domain is specified + $server['tablePrefix'] = $this->localDomain->getTablePrefix(); + } + $conn = $this->reallyOpenConnection( $server, $this->localDomain->getDatabase() ); $host = $this->getServerName( $i ); if ( $conn->isOpen() ) { $this->connLogger->debug( "Connected to database $i at '$host'." ); @@ -899,8 +896,6 @@ class LoadBalancer implements ILoadBalancer { // 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 ) ) { $this->mLastError = "Error selecting database '$dbName' on server " . $conn->getServer() . " from client host {$this->host}"; @@ -909,7 +904,8 @@ class LoadBalancer implements ILoadBalancer { } else { $conn->tablePrefix( $prefix ); unset( $this->mConns[$connFreeKey][$i][$oldDomain] ); - $this->mConns[$connInUseKey][$i][$domain] = $conn; + // Note that if $domain is an empty string, getDomainID() might not match it + $this->mConns[$connInUseKey][$i][$conn->getDomainId()] = $conn; $this->connLogger->debug( __METHOD__ . ": reusing free connection from $oldDomain for $domain" ); } @@ -929,8 +925,9 @@ class LoadBalancer implements ILoadBalancer { $this->errorConnection = $conn; $conn = false; } else { - $conn->tablePrefix( $prefix ); - $this->mConns[$connInUseKey][$i][$domain] = $conn; + $conn->tablePrefix( $prefix ); // as specified + // Note that if $domain is an empty string, getDomainID() might not match it + $this->mConns[$connInUseKey][$i][$conn->getDomainID()] = $conn; $this->connLogger->debug( __METHOD__ . ": opened new connection for $i/$domain" ); } } @@ -960,22 +957,22 @@ class LoadBalancer implements ILoadBalancer { } /** - * Really opens a connection. Uncached. + * Open a new network connection to a server (uncached) + * * Returns a Database object whether or not the connection was successful. - * @access private * * @param array $server - * @param string|bool $dbNameOverride Use "" to not select any database + * @param string|null $dbNameOverride Use "" to not select any database * @return Database * @throws DBAccessError * @throws InvalidArgumentException */ - protected function reallyOpenConnection( array $server, $dbNameOverride = false ) { + protected function reallyOpenConnection( array $server, $dbNameOverride ) { if ( $this->disabled ) { throw new DBAccessError(); } - if ( $dbNameOverride !== false ) { + if ( $dbNameOverride !== null ) { $server['dbname'] = $dbNameOverride; } @@ -1691,17 +1688,35 @@ class LoadBalancer implements ILoadBalancer { "Foreign domain connections are still in use ($domains)." ); } - $this->localDomain = new DatabaseDomain( + $oldDomain = $this->localDomain->getId(); + $this->setLocalDomain( new DatabaseDomain( $this->localDomain->getDatabase(), null, $prefix - ); + ) ); - $this->forEachOpenConnection( function ( IDatabase $db ) use ( $prefix ) { - $db->tablePrefix( $prefix ); + $this->forEachOpenConnection( function ( IDatabase $db ) use ( $prefix, $oldDomain ) { + if ( !$db->getLBInfo( 'foreign' ) ) { + $db->tablePrefix( $prefix ); + } } ); } + /** + * @param DatabaseDomain $domain + */ + private function setLocalDomain( DatabaseDomain $domain ) { + $this->localDomain = $domain; + // In case a caller assumes that the domain ID is simply -, which is almost + // always true, gracefully handle the case when they fail to account for escaping. + if ( $this->localDomain->getTablePrefix() != '' ) { + $this->localDomainIdAlias = + $this->localDomain->getDatabase() . '-' . $this->localDomain->getTablePrefix(); + } else { + $this->localDomainIdAlias = $this->localDomain->getDatabase(); + } + } + /** * Make PHP ignore user aborts/disconnects until the returned * value leaves scope. This returns null and does nothing in CLI mode. diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php b/includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php index 79d250f6a0..c73756330a 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php @@ -72,7 +72,7 @@ class LoadBalancerSingle extends LoadBalancer { return new static( [ 'connection' => $db ] + $params ); } - protected function reallyOpenConnection( array $server, $dbNameOverride = false ) { + protected function reallyOpenConnection( array $server, $dbNameOverride ) { return $this->db; } } diff --git a/tests/phpunit/includes/Storage/RevisionStoreDbTest.php b/tests/phpunit/includes/Storage/RevisionStoreDbTest.php index e33de20b2e..8185670206 100644 --- a/tests/phpunit/includes/Storage/RevisionStoreDbTest.php +++ b/tests/phpunit/includes/Storage/RevisionStoreDbTest.php @@ -44,7 +44,7 @@ class RevisionStoreDbTest extends MediaWikiTestCase { ->getMock(); $lb->method( 'reallyOpenConnection' )->willReturnCallback( - function ( array $server, $dbNameOverride = false ) { + function ( array $server, $dbNameOverride ) { return $this->getDatabaseMock( $server ); } ); diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php index e46a0ddbf9..e52dac6908 100644 --- a/tests/phpunit/includes/db/LBFactoryTest.php +++ b/tests/phpunit/includes/db/LBFactoryTest.php @@ -27,6 +27,7 @@ use Wikimedia\Rdbms\LBFactorySimple; use Wikimedia\Rdbms\LBFactoryMulti; use Wikimedia\Rdbms\ChronologyProtector; use Wikimedia\Rdbms\MySQLMasterPos; +use Wikimedia\Rdbms\DatabaseDomain; /** * @group Database @@ -314,7 +315,8 @@ class LBFactoryTest extends MediaWikiTestCase { } private function newLBFactoryMulti( array $baseOverride = [], array $serverOverride = [] ) { - global $wgDBserver, $wgDBuser, $wgDBpassword, $wgDBname, $wgDBtype, $wgSQLiteDataDir; + global $wgDBserver, $wgDBuser, $wgDBpassword, $wgDBname, $wgDBprefix, $wgDBtype; + global $wgSQLiteDataDir; return new LBFactoryMulti( $baseOverride + [ 'sectionsByDB' => [], @@ -325,6 +327,7 @@ class LBFactoryTest extends MediaWikiTestCase { ], 'serverTemplate' => $serverOverride + [ 'dbname' => $wgDBname, + 'tablePrefix' => $wgDBprefix, 'user' => $wgDBuser, 'password' => $wgDBpassword, 'type' => $wgDBtype, @@ -335,7 +338,7 @@ class LBFactoryTest extends MediaWikiTestCase { 'test-db1' => $wgDBserver, ], 'loadMonitorClass' => 'LoadMonitorNull', - 'localDomain' => wfWikiID() + 'localDomain' => new DatabaseDomain( $wgDBname, null, $wgDBprefix ) ] ); } @@ -361,7 +364,7 @@ class LBFactoryTest extends MediaWikiTestCase { if ( $wgDBtype !== 'sqlite' ) { $db = $lb->getConnectionRef( DB_MASTER ); $this->assertEquals( - $wgDBname, + wfWikiID(), $db->getDomainID() ); unset( $db ); @@ -369,34 +372,45 @@ class LBFactoryTest extends MediaWikiTestCase { /** @var Database $db */ $db = $lb->getConnection( DB_MASTER, [], '' ); - $lb->reuseConnection( $db ); // don't care + $this->assertEquals( + $wgDBname, + $db->getDomainId(), + 'Main domain ID handle used; same DB name' + ); + $this->assertEquals( + $wgDBname, + $db->getDBname(), + 'Main domain ID handle used; same DB name' + ); $this->assertEquals( '', - $db->getDomainID() + $db->tablePrefix(), + 'Main domain ID handle used; prefix is empty though' ); - $this->assertEquals( $this->quoteTable( $db, 'page' ), $db->tableName( 'page' ), "Correct full table name" ); - $this->assertEquals( $this->quoteTable( $db, $wgDBname ) . '.' . $this->quoteTable( $db, 'page' ), $db->tableName( "$wgDBname.page" ), "Correct full table name" ); - $this->assertEquals( $this->quoteTable( $db, 'nice_db' ) . '.' . $this->quoteTable( $db, 'page' ), $db->tableName( 'nice_db.page' ), "Correct full table name" ); + $lb->reuseConnection( $db ); // don't care + + $db = $lb->getConnection( DB_MASTER ); // local domain connection $factory->setDomainPrefix( 'my_' ); + $this->assertEquals( - '', + "$wgDBname-my_", $db->getDomainID() ); $this->assertEquals( @@ -415,7 +429,7 @@ class LBFactoryTest extends MediaWikiTestCase { } public function testTrickyDomain() { - global $wgDBtype; + global $wgDBtype, $wgDBname; if ( $wgDBtype === 'sqlite' ) { $tmpDir = $this->getNewTempDirectory(); @@ -427,18 +441,17 @@ class LBFactoryTest extends MediaWikiTestCase { $dbPath = null; } - $dbname = 'unittest-domain'; + $dbname = 'unittest-domain'; // explodes if DB is selected $factory = $this->newLBFactoryMulti( - [ 'localDomain' => $dbname ], - [ 'dbname' => $dbname, 'dbFilePath' => $dbPath ] + [ 'localDomain' => ( new DatabaseDomain( $dbname, null, '' ) )->getId() ], + [ 'dbFilePath' => $dbPath ] ); $lb = $factory->getMainLB(); /** @var Database $db */ $db = $lb->getConnection( DB_MASTER, [], '' ); - $lb->reuseConnection( $db ); // don't care $this->assertEquals( - '', + $wgDBname, $db->getDomainID() ); @@ -460,7 +473,10 @@ class LBFactoryTest extends MediaWikiTestCase { "Correct full table name" ); + $lb->reuseConnection( $db ); // don't care + $factory->setDomainPrefix( 'my_' ); + $db = $lb->getConnection( DB_MASTER, [], "$wgDBname-my_" ); $this->assertEquals( $this->quoteTable( $db, 'my_page' ), @@ -472,7 +488,6 @@ class LBFactoryTest extends MediaWikiTestCase { $db->tableName( 'other_nice_db.page' ), "Correct full table name" ); - $this->assertEquals( $this->quoteTable( $db, 'garbage-db' ) . '.' . $this->quoteTable( $db, 'page' ), $db->tableName( 'garbage-db.page' ), @@ -494,6 +509,8 @@ class LBFactoryTest extends MediaWikiTestCase { \MediaWiki\restoreWarnings(); } + $lb->reuseConnection( $db ); // don't care + $factory->closeAll(); $factory->destroy(); } diff --git a/tests/phpunit/includes/jobqueue/JobQueueTest.php b/tests/phpunit/includes/jobqueue/JobQueueTest.php index 7b34b59b15..bd21dc8a3e 100644 --- a/tests/phpunit/includes/jobqueue/JobQueueTest.php +++ b/tests/phpunit/includes/jobqueue/JobQueueTest.php @@ -1,5 +1,7 @@ 'JobQueueDB' ]; + $baseConfig = [ 'class' => 'JobQueueDBSingle' ]; } $baseConfig['type'] = 'null'; $baseConfig['wiki'] = wfWikiID(); @@ -381,3 +383,11 @@ class JobQueueTest extends MediaWikiTestCase { [ 'lives' => 0, 'usleep' => 0, 'removeDuplicates' => 1, 'i' => $i ] + $rootJob ); } } + +class JobQueueDBSingle extends JobQueueDB { + protected function getDB( $index ) { + $lb = MediaWikiServices::getInstance()->getDBLoadBalancer(); + // Override to not use CONN_TRX_AUTO so that we see the same temporary `job` table + return $lb->getConnection( $index, [], $this->wiki ); + } +} -- 2.20.1