Fixes to masterPosWait() for master switchovers

* Clean up return value types and docs.
* Handle master switch-over better w.r.t the job queue due
  to binlog name changes (the host portion). Previously the method
  would fail and trigger read-only mode when waiting on former
  master positions. Assume the the switch-over was done properly
  and thus return immediately.

Bug: T126436
Change-Id: Ib8c05a5c72d03a5c98e41b23c5653fc194b6d130

diff --git a/includes/db/DatabaseMysqlBase.php b/includes/db/DatabaseMysqlBase.php
index 7058061..1e27205 100644 (file)
--- a/includes/db/DatabaseMysqlBase.php
+++ b/includes/db/DatabaseMysqlBase.php
@@ -757,19 +757,13 @@ abstract class DatabaseMysqlBase extends Database {
                return $approxLag;
        }
 
-       /**
-        * Wait for the slave to catch up to a given master position.
-        * @todo Return values for this and base class are rubbish
-        *
-        * @param DBMasterPos|MySQLMasterPos $pos
-        * @param int $timeout The maximum number of seconds to wait for synchronisation
-        * @return int Zero if the slave was past that position already,
-        *   greater than zero if we waited for some period of time, less than
-        *   zero if we timed out.
-        */
        function masterPosWait( DBMasterPos $pos, $timeout ) {
+               if ( !( $pos instanceof MySQLMasterPos ) ) {
+                       throw new InvalidArgumentException( "Position not an instance of MySQLMasterPos" );
+               }
+
                if ( $this->lastKnownSlavePos && $this->lastKnownSlavePos->hasReached( $pos ) ) {
-                       return '0'; // http://dev.mysql.com/doc/refman/5.0/en/miscellaneous-functions.html
+                       return 0;
                }
 
                # Commit any open transactions
@@ -778,18 +772,28 @@ abstract class DatabaseMysqlBase extends Database {
                # Call doQuery() directly, to avoid opening a transaction if DBO_TRX is set
                $encFile = $this->addQuotes( $pos->file );
                $encPos = intval( $pos->pos );
-               $sql = "SELECT MASTER_POS_WAIT($encFile, $encPos, $timeout)";
-               $res = $this->doQuery( $sql );
-
-               $status = false;
-               if ( $res ) {
-                       $row = $this->fetchRow( $res );
-                       if ( $row ) {
-                               $status = $row[0]; // can be NULL, -1, or 0+ per the MySQL manual
-                               if ( ctype_digit( $status ) ) { // success
-                                       $this->lastKnownSlavePos = $pos;
-                               }
+               $res = $this->doQuery( "SELECT MASTER_POS_WAIT($encFile, $encPos, $timeout)" );
+
+               $row = $res ? $this->fetchRow( $res ) : false;
+               if ( !$row ) {
+                       throw new DBExpectedError( $this, "Failed to query MASTER_POS_WAIT()" );
+               }
+
+               // Result can be NULL (error), -1 (timeout), or 0+ per the MySQL manual
+               $status = ( $row[0] !== null ) ? intval( $row[0] ) : null;
+               if ( $status === null ) {
+                       // T126436: jobs programmed to wait on master positions might be referencing binlogs
+                       // with an old master hostname. Such calls make MASTER_POS_WAIT() return null. Try
+                       // to detect this and treat the slave as having reached the position; a proper master
+                       // switchover already requires that the new master be caught up before the switch.
+                       $slavePos = $this->getSlavePos();
+                       if ( $slavePos && !$slavePos->channelsMatch( $pos ) ) {
+                               $this->lastKnownSlavePos = $slavePos;
+                               $status = 0;
                        }
+               } elseif ( $status >= 0 ) {
+                       // Remember that this position was reached to save queries next time
+                       $this->lastKnownSlavePos = $pos;
                }
 
                return $status;
@@ -1446,11 +1450,34 @@ class MySQLMasterPos implements DBMasterPos {
                return ( $thisPos && $thatPos && $thisPos >= $thatPos );
        }
 
+       function channelsMatch( DBMasterPos $pos ) {
+               if ( !( $pos instanceof self ) ) {
+                       throw new InvalidArgumentException( "Position not an instance of " . __CLASS__ );
+               }
+
+               $thisBinlog = $this->getBinlogName();
+               $thatBinlog = $pos->getBinlogName();
+
+               return ( $thisBinlog !== false && $thisBinlog === $thatBinlog );
+       }
+
        function __toString() {
                // e.g db1034-bin.000976/843431247
                return "{$this->file}/{$this->pos}";
        }
 
+       /**
+        * @return string|bool
+        */
+       protected function getBinlogName() {
+               $m = [];
+               if ( preg_match( '!^(.+)\.(\d+)/(\d+)$!', (string)$this, $m ) ) {
+                       return $m[1];
+               }
+
+               return false;
+       }
+
        /**
         * @return array|bool (int, int)
         */

diff --git a/includes/db/DatabaseUtility.php b/includes/db/DatabaseUtility.php
index 6fa8bf0..b6c37ee 100644 (file)
--- a/includes/db/DatabaseUtility.php
+++ b/includes/db/DatabaseUtility.php
@@ -332,6 +332,13 @@ interface DBMasterPos {
         */
        public function hasReached( DBMasterPos $pos );
 
+       /**
+        * @param DBMasterPos $pos
+        * @return bool Whether this position appears to be for the same channel as another
+        * @since 1.27
+        */
+       public function channelsMatch( DBMasterPos $pos );
+
        /**
         * @return string
         * @since 1.27

diff --git a/includes/db/IDatabase.php b/includes/db/IDatabase.php
index e1d1173..225122d 100644 (file)
--- a/includes/db/IDatabase.php
+++ b/includes/db/IDatabase.php
@@ -1183,14 +1183,13 @@ interface IDatabase {
        public function wasReadOnlyError();
 
        /**
-        * Wait for the slave to catch up to a given master position.
+        * Wait for the slave to catch up to a given master position
         *
         * @param DBMasterPos $pos
-        * @param int $timeout The maximum number of seconds to wait for
-        *   synchronisation
-        * @return int Zero if the slave was past that position already,
+        * @param int $timeout The maximum number of seconds to wait for synchronisation
+        * @return int|null Zero if the slave was past that position already,
         *   greater than zero if we waited for some period of time, less than
-        *   zero if we timed out.
+        *   zero if it timed out, and null on error
         */
        public function masterPosWait( DBMasterPos $pos, $timeout );
 

diff --git a/tests/phpunit/includes/db/DatabaseMysqlBaseTest.php b/tests/phpunit/includes/db/DatabaseMysqlBaseTest.php
index 8e8002c..168b2c6 100644 (file)
--- a/tests/phpunit/includes/db/DatabaseMysqlBaseTest.php
+++ b/tests/phpunit/includes/db/DatabaseMysqlBaseTest.php
@@ -247,14 +247,64 @@ class DatabaseMysqlBaseTest extends MediaWikiTestCase {
                ];
        }
 
-       function testMasterPos() {
-               $pos1 = new MySQLMasterPos( 'db1034-bin.000976', '843431247' );
-               $pos2 = new MySQLMasterPos( 'db1034-bin.000976', '843431248' );
-
-               $this->assertTrue( $pos1->hasReached( $pos1 ) );
-               $this->assertTrue( $pos2->hasReached( $pos2 ) );
-               $this->assertTrue( $pos2->hasReached( $pos1 ) );
-               $this->assertFalse( $pos1->hasReached( $pos2 ) );
+       /**
+        * @dataProvider provideComparePositions
+        */
+       function testHasReached( MySQLMasterPos $lowerPos, MySQLMasterPos $higherPos ) {
+               $this->assertTrue( $higherPos->hasReached( $lowerPos ) );
+               $this->assertTrue( $higherPos->hasReached( $higherPos ) );
+               $this->assertTrue( $lowerPos->hasReached( $lowerPos ) );
+               $this->assertFalse( $lowerPos->hasReached( $higherPos ) );
+       }
+
+       function provideComparePositions() {
+               return [
+                       [
+                               new MySQLMasterPos( 'db1034-bin.000976', '843431247' ),
+                               new MySQLMasterPos( 'db1034-bin.000976', '843431248' )
+                       ],
+                       [
+                               new MySQLMasterPos( 'db1034-bin.000976', '999' ),
+                               new MySQLMasterPos( 'db1034-bin.000976', '1000' )
+                       ],
+                       [
+                               new MySQLMasterPos( 'db1034-bin.000976', '999' ),
+                               new MySQLMasterPos( 'db1035-bin.000976', '1000' )
+                       ],
+               ];
+       }
+
+       /**
+        * @dataProvider provideChannelPositions
+        */
+       function testChannelsMatch( MySQLMasterPos $pos1, MySQLMasterPos $pos2, $matches ) {
+               $this->assertEquals( $matches, $pos1->channelsMatch( $pos2 ) );
+               $this->assertEquals( $matches, $pos2->channelsMatch( $pos1 ) );
+       }
+
+       function provideChannelPositions() {
+               return [
+                       [
+                               new MySQLMasterPos( 'db1034-bin.000876', '44' ),
+                               new MySQLMasterPos( 'db1034-bin.000976', '74' ),
+                               true
+                       ],
+                       [
+                               new MySQLMasterPos( 'db1052-bin.000976', '999' ),
+                               new MySQLMasterPos( 'db1052-bin.000976', '1000' ),
+                               true
+                       ],
+                       [
+                               new MySQLMasterPos( 'db1066-bin.000976', '9999' ),
+                               new MySQLMasterPos( 'db1035-bin.000976', '10000' ),
+                               false
+                       ],
+                       [
+                               new MySQLMasterPos( 'db1066-bin.000976', '9999' ),
+                               new MySQLMasterPos( 'trump2016.000976', '10000' ),
+                               false
+                       ],
+               ];
        }
 
        /**