From 3675f1d447204dc07d87cefc9a6015e67f33798e Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 9 Aug 2016 19:15:05 -0700 Subject: [PATCH] Make Database disconnect and error suppression more robust * Disallow $ignoreErrors in query() on deadlocks, since that would otherwise silently rollback all changes from any other callers. * Move recoverability checks for disconnects to canRecoverFromDisconnect(). * The first write of a DBO_TRX transaction is now considered recoverable. * Run onTransactionResolution() callbacks on disconnect/deadlock rollback. Some DeferrableUpdate need this to know to abort. * Disallow $ignoreErrors on disconnects considered unrecoverable. This makes it so that query() callers cannot cause writes from other callers to be silently lost, which is hard to reason about. * Moved ping() logic to simple reconnect() method and ping() simply do a dummy SELECT, which triggeres reconnection if safe. Previously, ping() might cause subtle partial transaction loss. * Remove ping() from strencode(), which would cause partial transaction loss where it actually reached. * Remove mysqlPing() per https://bugs.php.net/bug.php?id=52561. Bug: T142079 Change-Id: Ifb7f772ae849d67c0d92240a115c3f392e252937 --- includes/db/Database.php | 108 ++++++++++++++---- includes/db/DatabaseMysql.php | 6 - includes/db/DatabaseMysqlBase.php | 29 +---- includes/db/DatabaseMysqli.php | 6 - includes/jobqueue/JobRunner.php | 2 +- .../includes/db/DatabaseMysqlBaseTest.php | 3 - 6 files changed, 89 insertions(+), 65 deletions(-) diff --git a/includes/db/Database.php b/includes/db/Database.php index c591f3ce4b..4e358d4c78 100644 --- a/includes/db/Database.php +++ b/includes/db/Database.php @@ -783,6 +783,7 @@ abstract class DatabaseBase implements IDatabase { public function query( $sql, $fname = __METHOD__, $tempIgnore = false ) { global $wgUser; + $priorWritesPending = $this->writesOrCallbacksPending(); $this->mLastQuery = $sql; $isWriteQuery = $this->isWriteQuery( $sql ); @@ -810,7 +811,10 @@ abstract class DatabaseBase implements IDatabase { // Or, for one-word queries (like "BEGIN" or COMMIT") add it to the end (bug 42598) $commentedSql = preg_replace( '/\s|$/', " /* $fname $userName */ ", $sql, 1 ); - if ( !$this->mTrxLevel && $this->getFlag( DBO_TRX ) && $this->isTransactableQuery( $sql ) ) { + # Start implicit transactions that wrap the request if DBO_TRX is enabled + if ( !$this->mTrxLevel && $this->getFlag( DBO_TRX ) + && $this->isTransactableQuery( $sql ) + ) { $this->begin( __METHOD__ . " ($fname)" ); $this->mTrxAutomatic = true; } @@ -862,31 +866,23 @@ abstract class DatabaseBase implements IDatabase { # Try reconnecting if the connection was lost if ( false === $ret && $this->wasErrorReissuable() ) { - # Transaction is gone; this can mean lost writes or REPEATABLE-READ snapshots - $hadTrx = $this->mTrxLevel; - # T127428: for non-write transactions, a disconnect and a COMMIT are similar: - # neither changed data and in both cases any read snapshots are reset anyway. - $isNoopCommit = ( !$this->writesOrCallbacksPending() && $sql === 'COMMIT' ); - # Update state tracking to reflect transaction loss - $this->mTrxLevel = 0; - $this->mTrxIdleCallbacks = []; // bug 65263 - $this->mTrxPreCommitCallbacks = []; // bug 65263 - wfDebug( "Connection lost, reconnecting...\n" ); - # Stash the last error values since ping() might clear them + $recoverable = $this->canRecoverFromDisconnect( $sql, $priorWritesPending ); + # Stash the last error values before anything might clear them $lastError = $this->lastError(); $lastErrno = $this->lastErrno(); - if ( $this->ping() ) { + # Update state tracking to reflect transaction loss due to disconnection + $this->handleTransactionLoss(); + wfDebug( "Connection lost, reconnecting...\n" ); + if ( $this->reconnect() ) { wfDebug( "Reconnected\n" ); - $server = $this->getServer(); - $msg = __METHOD__ . ": lost connection to $server; reconnected"; + $msg = __METHOD__ . ": lost connection to {$this->getServer()}; reconnected"; wfDebugLog( 'DBPerformance', "$msg:\n" . wfBacktrace( true ) ); - if ( ( $hadTrx && !$isNoopCommit ) || $this->mNamedLocksHeld ) { - # Leave $ret as false and let an error be reported. - # Callers may catch the exception and continue to use the DB. - $this->reportQueryError( $lastError, $lastErrno, $sql, $fname, $tempIgnore ); + if ( !$recoverable ) { + # Callers may catch the exception and continue to use the DB + $this->reportQueryError( $lastError, $lastErrno, $sql, $fname ); } else { - # Should be safe to silently retry (no trx/callbacks/locks) + # Should be safe to silently retry the query $startTime = microtime( true ); $ret = $this->doQuery( $commentedSql ); $queryRuntime = microtime( true ) - $startTime; @@ -900,6 +896,17 @@ abstract class DatabaseBase implements IDatabase { } if ( false === $ret ) { + # Deadlocks cause the entire transaction to abort, not just the statement. + # http://dev.mysql.com/doc/refman/5.7/en/innodb-error-handling.html + # https://www.postgresql.org/docs/9.1/static/explicit-locking.html + if ( $this->wasDeadlock() ) { + if ( $this->explicitTrxActive() || $priorWritesPending ) { + $tempIgnore = false; // not recoverable + } + # Update state tracking to reflect transaction loss + $this->handleTransactionLoss(); + } + $this->reportQueryError( $this->lastError(), $this->lastErrno(), $sql, $fname, $tempIgnore ); } @@ -918,6 +925,40 @@ abstract class DatabaseBase implements IDatabase { return $res; } + private function canRecoverFromDisconnect( $sql, $priorWritesPending ) { + # Transaction dropped; this can mean lost writes, or REPEATABLE-READ snapshots. + # Dropped connections also mean that named locks are automatically released. + # Only allow error suppression in autocommit mode or when the lost transaction + # didn't matter anyway (aside from DBO_TRX snapshot loss). + if ( $this->mNamedLocksHeld ) { + return false; // possible critical section violation + } elseif ( $sql === 'COMMIT' ) { + return !$priorWritesPending; // nothing written anyway? (T127428) + } elseif ( $sql === 'ROLLBACK' ) { + return true; // transaction lost...which is also what was requested :) + } elseif ( $this->explicitTrxActive() ) { + return false; // don't drop atomocity + } elseif ( $priorWritesPending ) { + return false; // prior writes lost from implicit transaction + } + + return true; + } + + private function handleTransactionLoss() { + $this->mTrxLevel = 0; + $this->mTrxIdleCallbacks = []; // bug 65263 + $this->mTrxPreCommitCallbacks = []; // bug 65263 + try { + // Handle callbacks in mTrxEndCallbacks + $this->runOnTransactionIdleCallbacks( self::TRIGGER_ROLLBACK ); + return null; + } catch ( Exception $e ) { + // Already logged; move on... + return $e; + } + } + public function reportQueryError( $error, $errno, $sql, $fname, $tempIgnore = false ) { if ( $this->ignoreErrors() || $tempIgnore ) { wfDebug( "SQL ERROR (ignored): $error\n" ); @@ -2509,6 +2550,7 @@ abstract class DatabaseBase implements IDatabase { * * @param integer $trigger IDatabase::TRIGGER_* constant * @since 1.20 + * @throws Exception */ public function runOnTransactionIdleCallbacks( $trigger ) { if ( $this->suppressPostCommitCallbacks ) { @@ -2516,7 +2558,7 @@ abstract class DatabaseBase implements IDatabase { } $autoTrx = $this->getFlag( DBO_TRX ); // automatic begin() enabled? - + /** @var Exception $e */ $e = $ePrior = null; // last exception do { // callbacks may add callbacks :) $callbacks = array_merge( @@ -2788,11 +2830,20 @@ abstract class DatabaseBase implements IDatabase { */ protected function doRollback( $fname ) { if ( $this->mTrxLevel ) { - $this->query( 'ROLLBACK', $fname, true ); + # Disconnects cause rollback anyway, so ignore those errors + $ignoreErrors = true; + $this->query( 'ROLLBACK', $fname, $ignoreErrors ); $this->mTrxLevel = 0; } } + /** + * @return bool + */ + protected function explicitTrxActive() { + return $this->mTrxLevel && ( $this->mTrxAtomicLevels || !$this->mTrxAutomatic ); + } + /** * Creates a new table with structure copied from existing table * Note that unlike most database abstraction functions, this function does not @@ -2894,6 +2945,19 @@ abstract class DatabaseBase implements IDatabase { } public function ping() { + try { + // This will reconnect if possible, or error out if not + $this->query( "SELECT 1 AS ping", __METHOD__ ); + return true; + } catch ( DBError $e ) { + return false; + } + } + + /** + * @return bool + */ + protected function reconnect() { # Stub. Not essential to override. return true; } diff --git a/includes/db/DatabaseMysql.php b/includes/db/DatabaseMysql.php index 5b15147777..87330b0e05 100644 --- a/includes/db/DatabaseMysql.php +++ b/includes/db/DatabaseMysql.php @@ -201,10 +201,4 @@ class DatabaseMysql extends DatabaseMysqlBase { return mysql_real_escape_string( $s, $conn ); } - - protected function mysqlPing() { - $conn = $this->getBindingHandle(); - - return mysql_ping( $conn ); - } } diff --git a/includes/db/DatabaseMysqlBase.php b/includes/db/DatabaseMysqlBase.php index 798815d4d1..6380fe7654 100644 --- a/includes/db/DatabaseMysqlBase.php +++ b/includes/db/DatabaseMysqlBase.php @@ -570,14 +570,7 @@ abstract class DatabaseMysqlBase extends Database { * @return string */ function strencode( $s ) { - $sQuoted = $this->mysqlRealEscapeString( $s ); - - if ( $sQuoted === false ) { - $this->ping(); - $sQuoted = $this->mysqlRealEscapeString( $s ); - } - - return $sQuoted; + return $this->mysqlRealEscapeString( $s ); } /** @@ -606,18 +599,7 @@ abstract class DatabaseMysqlBase extends Database { return strlen( $name ) && $name[0] == '`' && substr( $name, -1, 1 ) == '`'; } - /** - * @return bool - */ - function ping() { - $ping = $this->mysqlPing(); - if ( $ping ) { - // Connection was good or lost but reconnected... - // @note: mysqlnd (php 5.6+) does not support this (PHP bug 52561) - return true; - } - - // Try a full disconnect/reconnect cycle if ping() failed + function reconnect() { $this->closeConnection(); $this->mOpened = false; $this->mConn = false; @@ -626,13 +608,6 @@ abstract class DatabaseMysqlBase extends Database { return true; } - /** - * Ping a server connection or reconnect if there is no connection - * - * @return bool - */ - abstract protected function mysqlPing(); - function getLag() { if ( $this->getLagDetectionMethod() === 'pt-heartbeat' ) { return $this->getLagFromPtHeartbeat(); diff --git a/includes/db/DatabaseMysqli.php b/includes/db/DatabaseMysqli.php index d45805ad46..cb580cc0e6 100644 --- a/includes/db/DatabaseMysqli.php +++ b/includes/db/DatabaseMysqli.php @@ -309,12 +309,6 @@ class DatabaseMysqli extends DatabaseMysqlBase { return $conn->real_escape_string( $s ); } - protected function mysqlPing() { - $conn = $this->getBindingHandle(); - - return $conn->ping(); - } - /** * Give an id for the connection * diff --git a/includes/jobqueue/JobRunner.php b/includes/jobqueue/JobRunner.php index 98d7d12c4f..a132dc5204 100644 --- a/includes/jobqueue/JobRunner.php +++ b/includes/jobqueue/JobRunner.php @@ -534,7 +534,7 @@ class JobRunner implements LoggerAwareInterface { wfGetLBFactory()->forEachLB( function( LoadBalancer $lb ) use ( $fname ) { $lb->forEachOpenConnection( function( IDatabase $conn ) use ( $fname ) { if ( $conn->writesOrCallbacksPending() ) { - $conn->query( "SELECT 1", $fname ); + $conn->ping(); } } ); } ); diff --git a/tests/phpunit/includes/db/DatabaseMysqlBaseTest.php b/tests/phpunit/includes/db/DatabaseMysqlBaseTest.php index bb7eb793b7..bc7542aded 100644 --- a/tests/phpunit/includes/db/DatabaseMysqlBaseTest.php +++ b/tests/phpunit/includes/db/DatabaseMysqlBaseTest.php @@ -76,9 +76,6 @@ class FakeDatabaseMysqlBase extends DatabaseMysqlBase { protected function mysqlFetchField( $res, $n ) { } - protected function mysqlPing() { - } - protected function mysqlRealEscapeString( $s ) { } -- 2.20.1