From: Aaron Schulz Date: Wed, 17 Feb 2016 22:31:31 +0000 (-0800) Subject: Fixes to masterPosWait() for master switchovers X-Git-Tag: 1.31.0-rc.0~7718^2 X-Git-Url: http://git.cyclocoop.org/%24image?a=commitdiff_plain;h=b64a178a2178f6393504fd0a589f4152158d9dac;p=lhc%2Fweb%2Fwiklou.git 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 7058061f8e..1e2720504c 100644 --- 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 6fa8bf09f3..b6c37ee7b6 100644 --- 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 e1d1173b14..225122dd7f 100644 --- 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 8e8002cee0..168b2c6837 100644 --- 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 + ], + ]; } /**