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 24b5402..8615cfc 100644 (file)
--- 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 d94d24d..a85e1e5 100644 (file)
--- 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 ba0b4a0..4a4e1bc 100644 (file)
--- 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 184cb0e..cab0201 100644 (file)
--- 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 123b080..424c64b 100644 (file)
--- 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 f1bcd98..8510109 100644 (file)
--- 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 ) );
+       }
 }