From d8732ad042a2842961e3c87de96accf31f31a9f3 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Tue, 20 Mar 2018 04:16:41 -0700 Subject: [PATCH] rdbms: clean up session/transaction loss logic in Database * Refactor the code to be less cluttered, moving some logic to reconnect(), which is now named replaceLostConnection() * Handle the case of a second connection loss on query retry * Make canRecoverFromDisconnect() check for temporary tables * Do not clear session-level variables on deadlock errors since only the transaction is lost, not the whole session * Make sure any empty transaction is destroyed on deadlock * Attempt reconnection *before* triggering the transaction callbacks since some of them might want to use the database * Define wasConnectionError() for postgres * Remove unused return value from handleSessionLoss() Change-Id: Ic1dcab03f087ce310637210e8e9bc0771e44f045 --- includes/libs/rdbms/database/Database.php | 110 ++++++++++-------- .../libs/rdbms/database/DatabasePostgres.php | 7 ++ 2 files changed, 71 insertions(+), 46 deletions(-) diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 211566e87c..25938f1150 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -1067,38 +1067,31 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware # Avoid fatals if close() was called $this->assertOpen(); - # Send the query to the server + # Send the query to the server and fetch any corresponding errors $ret = $this->doProfiledQuery( $sql, $commentedSql, $isNonTempWrite, $fname ); + $lastError = $this->lastError(); + $lastErrno = $this->lastErrno(); # Try reconnecting if the connection was lost - if ( false === $ret && $this->wasConnectionLoss() ) { + if ( $ret === false && $this->wasConnectionLoss() ) { + # Check if any meaningful session state was lost $recoverable = $this->canRecoverFromDisconnect( $sql, $priorWritesPending ); - # Stash the last error values before anything might clear them - $lastError = $this->lastError(); - $lastErrno = $this->lastErrno(); - # Update state tracking to reflect transaction loss due to disconnection - $this->handleSessionLoss(); - if ( $this->reconnect() ) { - $msg = __METHOD__ . ': lost connection to {dbserver}; reconnected'; - $params = [ 'dbserver' => $this->getServer() ]; - $this->connLogger->warning( $msg, $params ); - $this->queryLogger->warning( $msg, $params + - [ 'trace' => ( new RuntimeException() )->getTraceAsString() ] ); - - if ( $recoverable ) { - # Should be safe to silently retry the query - $ret = $this->doProfiledQuery( $sql, $commentedSql, $isNonTempWrite, $fname ); - } else { - # Callers may catch the exception and continue to use the DB - $this->reportQueryError( $lastError, $lastErrno, $sql, $fname ); + # Update session state tracking and try to restore the connection + $reconnected = $this->replaceLostConnection( __METHOD__ ); + # Silently resend the query to the server if it is safe and possible + if ( $reconnected && $recoverable ) { + $ret = $this->doProfiledQuery( $sql, $commentedSql, $isNonTempWrite, $fname ); + $lastError = $this->lastError(); + $lastErrno = $this->lastErrno(); + + if ( $ret === false && $this->wasConnectionLoss() ) { + # Query probably causes disconnects; reconnect and do not re-run it + $this->replaceLostConnection( __METHOD__ ); } - } else { - $msg = __METHOD__ . ': lost connection to {dbserver} permanently'; - $this->connLogger->error( $msg, [ 'dbserver' => $this->getServer() ] ); } } - if ( false === $ret ) { + if ( $ret === false ) { # Deadlocks cause the entire transaction to abort, not just the statement. # https://dev.mysql.com/doc/refman/5.7/en/innodb-error-handling.html # https://www.postgresql.org/docs/9.1/static/explicit-locking.html @@ -1106,17 +1099,19 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( $this->explicitTrxActive() || $priorWritesPending ) { $tempIgnore = false; // not recoverable } + # Usually the transaction is rolled back to BEGIN, leaving an empty transaction. + # Destroy any such transaction so the rollback callbacks run in AUTO-COMMIT mode + # as normal. Also, if DBO_TRX is set and an explicit transaction rolled back here, + # further queries should be back in AUTO-COMMIT mode, not stuck in a transaction. + $this->doRollback( __METHOD__ ); # Update state tracking to reflect transaction loss - $this->handleSessionLoss(); + $this->handleTransactionLoss(); } - $this->reportQueryError( - $this->lastError(), $this->lastErrno(), $sql, $fname, $tempIgnore ); + $this->reportQueryError( $lastError, $lastErrno, $sql, $fname, $tempIgnore ); } - $res = $this->resultObject( $ret ); - - return $res; + return $this->resultObject( $ret ); } /** @@ -1231,6 +1226,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware # didn't matter anyway (aside from DBO_TRX snapshot loss). if ( $this->namedLocksHeld ) { return false; // possible critical section violation + } elseif ( $this->sessionTempTables ) { + return false; // tables might be queried latter } elseif ( $sql === 'COMMIT' ) { return !$priorWritesPending; // nothing written anyway? (T127428) } elseif ( $sql === 'ROLLBACK' ) { @@ -1245,36 +1242,41 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } /** - * Clean things up after transaction loss due to disconnection - * - * @return null|Exception + * Clean things up after session (and thus transaction) loss */ private function handleSessionLoss() { + // Clean up tracking of session-level things... + // https://dev.mysql.com/doc/refman/5.7/en/implicit-commit.html + // https://www.postgresql.org/docs/9.1/static/sql-createtable.html (ignoring ON COMMIT) + $this->sessionTempTables = []; + // https://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_get-lock + // https://www.postgresql.org/docs/9.4/static/functions-admin.html#FUNCTIONS-ADVISORY-LOCKS + $this->namedLocksHeld = []; + // Session loss implies transaction loss + $this->handleTransactionLoss(); + } + + /** + * Clean things up after transaction loss + */ + private function handleTransactionLoss() { $this->trxLevel = 0; $this->trxAtomicCounter = 0; $this->trxIdleCallbacks = []; // T67263; transaction already lost $this->trxPreCommitCallbacks = []; // T67263; transaction already lost - $this->sessionTempTables = []; - $this->namedLocksHeld = []; - - // Note: if callback suppression is set then some *Callbacks arrays are not cleared here - $e = null; try { - // Handle callbacks in trxEndCallbacks + // Handle callbacks in trxEndCallbacks, e.g. onTransactionResolution(). + // If callback suppression is set then the array will remain unhandled. $this->runOnTransactionIdleCallbacks( self::TRIGGER_ROLLBACK ); } catch ( Exception $ex ) { // Already logged; move on... - $e = $e ?: $ex; } try { - // Handle callbacks in trxRecurringCallbacks + // Handle callbacks in trxRecurringCallbacks, e.g. setTransactionListener() $this->runTransactionListenerCallbacks( self::TRIGGER_ROLLBACK ); } catch ( Exception $ex ) { // Already logged; move on... - $e = $e ?: $ex; } - - return $e; } /** @@ -3671,11 +3673,12 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } /** - * Close existing database connection and open a new connection + * Close any existing (dead) database connection and open a new connection * + * @param string $fname * @return bool True if new connection is opened successfully, false if error */ - protected function reconnect() { + protected function replaceLostConnection( $fname ) { $this->closeConnection(); $this->opened = false; $this->conn = false; @@ -3683,10 +3686,25 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->open( $this->server, $this->user, $this->password, $this->dbName ); $this->lastPing = microtime( true ); $ok = true; + + $this->connLogger->warning( + $fname . ': lost connection to {dbserver}; reconnected', + [ + 'dbserver' => $this->getServer(), + 'trace' => ( new RuntimeException() )->getTraceAsString() + ] + ); } catch ( DBConnectionError $e ) { $ok = false; + + $this->connLogger->error( + $fname . ': lost connection to {dbserver} permanently', + [ 'dbserver' => $this->getServer() ] + ); } + $this->handleSessionLoss(); + return $ok; } diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index 13d9158725..e3dd3d00b3 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -814,6 +814,13 @@ __INDEXATTR__; return $this->lastErrno() === '55P03'; } + public function wasConnectionError( $errno ) { + // https://www.postgresql.org/docs/8.2/static/errcodes-appendix.html + static $codes = [ '08000', '08003', '08006', '08001', '08004', '57P01', '57P03', '53300' ]; + + return in_array( $errno, $codes, true ); + } + public function duplicateTableStructure( $oldName, $newName, $temporary = false, $fname = __METHOD__ ) { -- 2.20.1