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

diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
index e80b952..b01b23f 100644 (file)
--- 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 <db>-<prefix>, 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 <db>-<prefix>, 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 79d250f..c737563 100644 (file)
--- 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 e33de20..8185670 100644 (file)
--- 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 e46a0dd..e52dac6 100644 (file)
--- 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 7b34b59..bd21dc8 100644 (file)
--- a/tests/phpunit/includes/jobqueue/JobQueueTest.php
+++ b/tests/phpunit/includes/jobqueue/JobQueueTest.php
@@ -1,5 +1,7 @@
 <?php
 
+use MediaWiki\MediaWikiServices;
+
 /**
  * @group JobQueue
  * @group medium
@@ -26,7 +28,7 @@ class JobQueueTest extends MediaWikiTestCase {
                        }
                        $baseConfig = $wgJobTypeConf[$name];
                } else {
-                       $baseConfig = [ 'class' => '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 );
+       }
+}