From: Aaron Schulz Date: Tue, 18 Jun 2019 23:57:32 +0000 (+0100) Subject: rdbms: add ILoadBalancer::getReplicaResumePos method X-Git-Tag: 1.34.0-rc.0~1038 X-Git-Url: http://git.cyclocoop.org/%7B%24www_url%7Dadmin/compta/operations/?a=commitdiff_plain;h=318306ed1771d7ce6b6b8d7042ac0f3e3330eede;p=lhc%2Fweb%2Fwiklou.git rdbms: add ILoadBalancer::getReplicaResumePos method This does what ChronologyProtector wants more rigorously and is better named. Not all replica servers will have the same position, so they should be compared to get the highest one. Simplify the getMasterPos() method to only return master positions as the other current callers do not need anything else. It will now connect if needed as well. This should make the method naming better. Reducing the use of replica derived replication postitions (instead of those from the master) makes certain GTID issues less likely, such as the matter of obsolete domain IDs. Increase general test coverage of LoadBalancer. Bug: T224422 Change-Id: I5420721ee339a24d09c26c38709500c7bbe797c2 --- diff --git a/includes/libs/rdbms/ChronologyProtector.php b/includes/libs/rdbms/ChronologyProtector.php index 24b54026cd..8615cfc630 100644 --- a/includes/libs/rdbms/ChronologyProtector.php +++ b/includes/libs/rdbms/ChronologyProtector.php @@ -177,7 +177,7 @@ class ChronologyProtector implements LoggerAwareInterface { $masterName = $lb->getServerName( $lb->getWriterIndex() ); if ( $lb->hasStreamingReplicaServers() ) { - $pos = $lb->getMasterPos(); + $pos = $lb->getReplicaResumePos(); if ( $pos ) { $this->logger->debug( __METHOD__ . ": LB for '$masterName' has pos $pos\n" ); $this->shutdownPositions[$masterName] = $pos; diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index d94d24d88e..a85e1e544f 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -422,14 +422,20 @@ abstract class LBFactory implements ILBFactory { // time needed to wait on the next clusters. $masterPositions = array_fill( 0, count( $lbs ), false ); foreach ( $lbs as $i => $lb ) { - if ( !$lb->hasStreamingReplicaServers() ) { - continue; // T29975: no replication; avoid getMasterPos() permissions errors - } elseif ( - $opts['ifWritesSince'] && - $lb->lastMasterChangeTimestamp() < $opts['ifWritesSince'] + if ( + // No writes to wait on getting replicated + !$lb->hasMasterConnection() || + // No replication; avoid getMasterPos() permissions errors (T29975) + !$lb->hasStreamingReplicaServers() || + // No writes since the last replication wait + ( + $opts['ifWritesSince'] && + $lb->lastMasterChangeTimestamp() < $opts['ifWritesSince'] + ) ) { - continue; // no writes since the last wait + continue; // no need to wait } + $masterPositions[$i] = $lb->getMasterPos(); } diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php index ba0b4a04e5..4a4e1bc9ed 100644 --- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php @@ -455,11 +455,30 @@ interface ILoadBalancer { public function getServerAttributes( $i ); /** - * Get the current master position for chronology control purposes + * Get the current master replication position + * * @return DBMasterPos|bool Returns false if not applicable + * @throws DBError */ public function getMasterPos(); + /** + * Get the highest DB replication position for chronology control purposes + * + * If there is only a master server then this returns false. If replication is present + * and correctly configured, then this returns the highest replication position of any + * server with an open connection. That position can later be passed to waitFor() on a + * new load balancer instance to make sure that queries on the new connections see data + * at least as up-to-date as queries (prior to this method call) on the old connections. + * + * This can be useful for implementing session consistency, where the session + * will be resumed accross multiple HTTP requests or CLI script instances. + * + * @return DBMasterPos|bool Replication position or false if not applicable + * @since 1.34 + */ + public function getReplicaResumePos(); + /** * Disable this load balancer. All connections are closed, and any attempt to * open a new connection will result in a DBAccessError. diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index 184cb0e54b..cab0201ed4 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -1452,22 +1452,56 @@ class LoadBalancer implements ILoadBalancer { } public function getMasterPos() { - # If this entire request was served from a replica DB without opening a connection to the - # master (however unlikely that may be), then we can fetch the position from the replica DB. + $index = $this->getWriterIndex(); + + $conn = $this->getAnyOpenConnection( $index ); + if ( $conn ) { + return $conn->getMasterPos(); + } + + $conn = $this->getConnection( $index, self::CONN_SILENCE_ERRORS ); + if ( !$conn ) { + $this->reportConnectionError(); + return null; // unreachable due to exception + } + + try { + $pos = $conn->getMasterPos(); + } finally { + $this->closeConnection( $conn ); + } + + return $pos; + } + + public function getReplicaResumePos() { + // Get the position of any existing master server connection $masterConn = $this->getAnyOpenConnection( $this->getWriterIndex() ); - if ( !$masterConn ) { - $serverCount = $this->getServerCount(); - for ( $i = 1; $i < $serverCount; $i++ ) { - $conn = $this->getAnyOpenConnection( $i ); - if ( $conn ) { - return $conn->getReplicaPos(); - } - } - } else { + if ( $masterConn ) { return $masterConn->getMasterPos(); } - return false; + // Get the highest position of any existing replica server connection + $highestPos = false; + $serverCount = $this->getServerCount(); + for ( $i = 1; $i < $serverCount; $i++ ) { + if ( !empty( $this->servers[$i]['is static'] ) ) { + continue; // server does not use replication + } + + $conn = $this->getAnyOpenConnection( $i ); + $pos = $conn ? $conn->getReplicaPos() : false; + if ( !$pos ) { + continue; // no open connection or could not get position + } + + $highestPos = $highestPos ?: $pos; + if ( $pos->hasReached( $highestPos ) ) { + $highestPos = $pos; + } + } + + return $highestPos; } public function disable() { diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php index 123b080b9a..424c64b094 100644 --- a/tests/phpunit/includes/db/LBFactoryTest.php +++ b/tests/phpunit/includes/db/LBFactoryTest.php @@ -315,6 +315,7 @@ class LBFactoryTest extends MediaWikiTestCase { } ) ); $lb1->method( 'getMasterPos' )->willReturn( $m1Pos ); + $lb1->method( 'getReplicaResumePos' )->willReturn( $m1Pos ); $lb1->method( 'getServerName' )->with( 0 )->willReturn( 'master1' ); // Master DB 2 $mockDB2 = $this->getMockBuilder( IDatabase::class ) @@ -342,6 +343,7 @@ class LBFactoryTest extends MediaWikiTestCase { } ) ); $lb2->method( 'getMasterPos' )->willReturn( $m2Pos ); + $lb2->method( 'getReplicaResumePos' )->willReturn( $m2Pos ); $lb2->method( 'getServerName' )->with( 0 )->willReturn( 'master2' ); $bag = new HashBagOStuff(); diff --git a/tests/phpunit/includes/db/LoadBalancerTest.php b/tests/phpunit/includes/db/LoadBalancerTest.php index f1bcd9898f..85101091a6 100644 --- a/tests/phpunit/includes/db/LoadBalancerTest.php +++ b/tests/phpunit/includes/db/LoadBalancerTest.php @@ -51,8 +51,11 @@ class LoadBalancerTest extends MediaWikiTestCase { } /** - * @covers LoadBalancer::getLocalDomainID() - * @covers LoadBalancer::resolveDomainID() + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnection() + * @covers \Wikimedia\Rdbms\LoadBalancer::getLocalDomainID() + * @covers \Wikimedia\Rdbms\LoadBalancer::resolveDomainID() + * @covers \Wikimedia\Rdbms\LoadBalancer::haveIndex() + * @covers \Wikimedia\Rdbms\LoadBalancer::isNonZeroLoad() */ public function testWithoutReplica() { global $wgDBname; @@ -68,6 +71,15 @@ class LoadBalancerTest extends MediaWikiTestCase { } ] ); + $this->assertEquals( 1, $lb->getServerCount() ); + $this->assertFalse( $lb->hasReplicaServers() ); + $this->assertFalse( $lb->hasStreamingReplicaServers() ); + + $this->assertTrue( $lb->haveIndex( 0 ) ); + $this->assertFalse( $lb->haveIndex( 1 ) ); + $this->assertFalse( $lb->isNonZeroLoad( 0 ) ); + $this->assertFalse( $lb->isNonZeroLoad( 1 ) ); + $ld = DatabaseDomain::newFromId( $lb->getLocalDomainID() ); $this->assertEquals( $wgDBname, $ld->getDatabase(), 'local domain DB set' ); $this->assertEquals( $this->dbPrefix(), $ld->getTablePrefix(), 'local domain prefix set' ); @@ -108,6 +120,17 @@ class LoadBalancerTest extends MediaWikiTestCase { $lb->closeAll(); } + /** + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnection() + * @covers \Wikimedia\Rdbms\LoadBalancer::getReaderIndex() + * @covers \Wikimedia\Rdbms\LoadBalancer::getWriterIndex() + * @covers \Wikimedia\Rdbms\LoadBalancer::haveIndex() + * @covers \Wikimedia\Rdbms\LoadBalancer::isNonZeroLoad() + * @covers \Wikimedia\Rdbms\LoadBalancer::getServerName() + * @covers \Wikimedia\Rdbms\LoadBalancer::getServerInfo() + * @covers \Wikimedia\Rdbms\LoadBalancer::getServerType() + * @covers \Wikimedia\Rdbms\LoadBalancer::getServerAttributes() + */ public function testWithReplica() { global $wgDBserver; @@ -118,6 +141,18 @@ class LoadBalancerTest extends MediaWikiTestCase { $this->assertTrue( $lb->hasReplicaServers() ); $this->assertTrue( $lb->hasStreamingReplicaServers() ); + $this->assertTrue( $lb->haveIndex( 0 ) ); + $this->assertTrue( $lb->haveIndex( 1 ) ); + $this->assertFalse( $lb->isNonZeroLoad( 0 ) ); + $this->assertTrue( $lb->isNonZeroLoad( 1 ) ); + + for ( $i = 0; $i < $lb->getServerCount(); ++$i ) { + $this->assertType( 'string', $lb->getServerName( $i ) ); + $this->assertType( 'array', $lb->getServerInfo( $i ) ); + $this->assertType( 'string', $lb->getServerType( $i ) ); + $this->assertType( 'array', $lb->getServerAttributes( $i ) ); + } + $dbw = $lb->getConnection( DB_MASTER ); $this->assertTrue( $dbw->getLBInfo( 'master' ), 'master shows as master' ); $this->assertEquals( @@ -136,6 +171,7 @@ class LoadBalancerTest extends MediaWikiTestCase { 'cluster master set' ); $this->assertTrue( $dbr->getFlag( $dbw::DBO_TRX ), "DBO_TRX set on replica" ); $this->assertWriteForbidden( $dbr ); + $this->assertEquals( $dbr->getLBInfo( 'serverIndex' ), $lb->getReaderIndex() ); if ( !$lb->getServerAttributes( $lb->getWriterIndex() )[$dbw::ATTR_DB_LEVEL_LOCKING] ) { $dbwAuto = $lb->getConnection( DB_MASTER, [], false, $lb::CONN_TRX_AUTOCOMMIT ); @@ -389,8 +425,10 @@ class LoadBalancerTest extends MediaWikiTestCase { } /** - * @covers LoadBalancer::openConnection() - * @covers LoadBalancer::getAnyOpenConnection() + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnection() + * @covers \Wikimedia\Rdbms\LoadBalancer::openConnection() + * @covers \Wikimedia\Rdbms\LoadBalancer::getAnyOpenConnection() + * @covers \Wikimedia\Rdbms\LoadBalancer::getWriterIndex() */ function testOpenConnection() { $lb = $this->newSingleServerLocalLoadBalancer(); @@ -436,6 +474,18 @@ class LoadBalancerTest extends MediaWikiTestCase { $lb->closeAll(); } + /** + * @covers \Wikimedia\Rdbms\LoadBalancer::openConnection() + * @covers \Wikimedia\Rdbms\LoadBalancer::getWriterIndex() + * @covers \Wikimedia\Rdbms\LoadBalancer::forEachOpenMasterConnection() + * @covers \Wikimedia\Rdbms\LoadBalancer::setTransactionListener() + * @covers \Wikimedia\Rdbms\LoadBalancer::beginMasterChanges() + * @covers \Wikimedia\Rdbms\LoadBalancer::finalizeMasterChanges() + * @covers \Wikimedia\Rdbms\LoadBalancer::approveMasterChanges() + * @covers \Wikimedia\Rdbms\LoadBalancer::commitMasterChanges() + * @covers \Wikimedia\Rdbms\LoadBalancer::runMasterTransactionIdleCallbacks() + * @covers \Wikimedia\Rdbms\LoadBalancer::runMasterTransactionListenerCallbacks() + */ public function testTransactionCallbackChains() { global $wgDBserver, $wgDBname, $wgDBuser, $wgDBpassword, $wgDBtype, $wgSQLiteDataDir; @@ -523,6 +573,10 @@ class LoadBalancerTest extends MediaWikiTestCase { $conn2->close(); } + /** + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnectionRef + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnection() + */ public function testDBConnRefReadsMasterAndReplicaRoles() { $lb = $this->newSingleServerLocalLoadBalancer(); @@ -547,6 +601,7 @@ class LoadBalancerTest extends MediaWikiTestCase { } /** + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnectionRef * @expectedException \Wikimedia\Rdbms\DBReadOnlyRoleError */ public function testDBConnRefWritesReplicaRole() { @@ -558,6 +613,7 @@ class LoadBalancerTest extends MediaWikiTestCase { } /** + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnectionRef * @expectedException \Wikimedia\Rdbms\DBReadOnlyRoleError */ public function testDBConnRefWritesReplicaRoleIndex() { @@ -569,6 +625,7 @@ class LoadBalancerTest extends MediaWikiTestCase { } /** + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnectionRef * @expectedException \Wikimedia\Rdbms\DBReadOnlyRoleError */ public function testDBConnRefWritesReplicaRoleInsert() { @@ -579,6 +636,10 @@ class LoadBalancerTest extends MediaWikiTestCase { $rConn->insert( 'test', [ 't' => 1 ], __METHOD__ ); } + /** + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnection() + * @covers \Wikimedia\Rdbms\LoadBalancer::getMaintenanceConnectionRef() + */ public function testQueryGroupIndex() { $lb = $this->newMultiServerLocalLoadBalancer( [ 'defaultGroup' => false ] ); /** @var LoadBalancer $lbWrapper */ @@ -599,9 +660,13 @@ class LoadBalancerTest extends MediaWikiTestCase { $rRC = $lb->getConnectionRef( DB_REPLICA, [ 'recentchanges' ] ); $rWL = $lb->getConnectionRef( DB_REPLICA, [ 'watchlist' ] ); + $rRCMaint = $lb->getMaintenanceConnectionRef( DB_REPLICA, [ 'recentchanges' ] ); + $rWLMaint = $lb->getMaintenanceConnectionRef( DB_REPLICA, [ 'watchlist' ] ); $this->assertEquals( 3, $rRC->getLBInfo( 'serverIndex' ) ); $this->assertEquals( 3, $rWL->getLBInfo( 'serverIndex' ) ); + $this->assertEquals( 3, $rRCMaint->getLBInfo( 'serverIndex' ) ); + $this->assertEquals( 3, $rWLMaint->getLBInfo( 'serverIndex' ) ); $rLog = $lb->getConnectionRef( DB_REPLICA, [ 'logging', 'watchlist' ] ); $logIndexPicked = $rLog->getLBInfo( 'serverIndex' ); @@ -628,4 +693,30 @@ class LoadBalancerTest extends MediaWikiTestCase { $rGeneric = $lb->getConnectionRef( DB_REPLICA ); $this->assertEquals( $lb->getWriterIndex(), $rGeneric->getLBInfo( 'serverIndex' ) ); } + + /** + * @covers \Wikimedia\Rdbms\LoadBalancer::getLazyConnectionRef + */ + public function testGetLazyConnectionRef() { + $lb = $this->newMultiServerLocalLoadBalancer(); + + $rMaster = $lb->getLazyConnectionRef( DB_MASTER ); + $rReplica = $lb->getLazyConnectionRef( 1 ); + $this->assertFalse( $lb->getAnyOpenConnection( 0 ) ); + $this->assertFalse( $lb->getAnyOpenConnection( 1 ) ); + + $rMaster->getType(); + $rReplica->getType(); + $rMaster->getDomainID(); + $rReplica->getDomainID(); + $this->assertFalse( $lb->getAnyOpenConnection( 0 ) ); + $this->assertFalse( $lb->getAnyOpenConnection( 1 ) ); + + $rMaster->query( "SELECT 1", __METHOD__ ); + $this->assertNotFalse( $lb->getAnyOpenConnection( 0 ) ); + + $rReplica->query( "SELECT 1", __METHOD__ ); + $this->assertNotFalse( $lb->getAnyOpenConnection( 0 ) ); + $this->assertNotFalse( $lb->getAnyOpenConnection( 1 ) ); + } }