From 8359993708fcd6668f427b9bf7bafbd5bf18935b Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 19 Aug 2016 23:51:46 -0700 Subject: [PATCH] Various database class cleanups * Refactor out some code duplication in query() into a separate private method. * Remove the total master/slave query profiling, which is not necessary and redundant. * Provide a default implementation for reconnect(). * Make reconnect() catch errors so it can match the docs that say it returns true/false to indicate failure. Likewise for ping(). * Optimize ping() to no-op if there was obvious recent activity. * Move the ping() round in JobRunner to approveMasterChanges. This way, all commit rounds benefit from this logic. * Add more doc comments for DatabaseBase fields. Change-Id: Ic90ce2be4187244a0e8d44854c39d4b78be8e642 --- includes/db/Database.php | 140 ++++++++++++++-------- includes/db/DatabaseMysqlBase.php | 9 -- includes/db/loadbalancer/LoadBalancer.php | 10 +- includes/jobqueue/JobRunner.php | 11 -- 4 files changed, 96 insertions(+), 74 deletions(-) diff --git a/includes/db/Database.php b/includes/db/Database.php index 186a87bb53..6ddc9f7a41 100644 --- a/includes/db/Database.php +++ b/includes/db/Database.php @@ -32,24 +32,35 @@ abstract class DatabaseBase implements IDatabase { /** Number of times to re-try an operation in case of deadlock */ const DEADLOCK_TRIES = 4; - /** Minimum time to wait before retry, in microseconds */ const DEADLOCK_DELAY_MIN = 500000; - /** Maximum time to wait before retry */ const DEADLOCK_DELAY_MAX = 1500000; + /** How long before it is worth doing a dummy query to test the connection */ + const PING_TTL = 1.0; + + /** @var string SQL query */ protected $mLastQuery = ''; + /** @var bool */ protected $mDoneWrites = false; + /** @var string|bool */ protected $mPHPError = false; - - protected $mServer, $mUser, $mPassword, $mDBname; + /** @var string */ + protected $mServer; + /** @var string */ + protected $mUser; + /** @var string */ + protected $mPassword; + /** @var string */ + protected $mDBname; /** @var BagOStuff APC cache */ protected $srvCache; /** @var resource Database connection */ protected $mConn = null; + /** @var bool */ protected $mOpened = false; /** @var array[] List of (callable, method name) */ @@ -61,20 +72,27 @@ abstract class DatabaseBase implements IDatabase { /** @var bool Whether to suppress triggering of post-commit callbacks */ protected $suppressPostCommitCallbacks = false; + /** @var string */ protected $mTablePrefix; + /** @var string */ protected $mSchema; + /** @var integer */ protected $mFlags; + /** @var bool */ protected $mForeign; + /** @var array */ protected $mLBInfo = []; + /** @var bool|null */ protected $mDefaultBigSelects = null; + /** @var array|bool */ protected $mSchemaVars = false; /** @var array */ protected $mSessionVars = []; - + /** @var array|null */ protected $preparedArgs; - + /** @var string|bool|null Stashed value of html_errors INI setting */ protected $htmlErrors; - + /** @var string */ protected $delimiter = ';'; /** @@ -177,6 +195,9 @@ abstract class DatabaseBase implements IDatabase { */ protected $allViews = null; + /** @var float UNIX timestamp */ + protected $lastPing = 0.0; + /** @var TransactionProfiler */ protected $trxProfiler; @@ -786,8 +807,8 @@ abstract class DatabaseBase implements IDatabase { $priorWritesPending = $this->writesOrCallbacksPending(); $this->mLastQuery = $sql; - $isWriteQuery = $this->isWriteQuery( $sql ); - if ( $isWriteQuery ) { + $isWrite = $this->isWriteQuery( $sql ); + if ( $isWrite ) { $reason = $this->getReadOnlyReason(); if ( $reason !== false ) { throw new DBReadOnlyError( $this, "Database is read-only: $reason" ); @@ -820,31 +841,12 @@ abstract class DatabaseBase implements IDatabase { } # Keep track of whether the transaction has write queries pending - if ( $this->mTrxLevel && !$this->mTrxDoneWrites && $isWriteQuery ) { + if ( $this->mTrxLevel && !$this->mTrxDoneWrites && $isWrite ) { $this->mTrxDoneWrites = true; $this->getTransactionProfiler()->transactionWritingIn( $this->mServer, $this->mDBname, $this->mTrxShortId ); } - $isMaster = !is_null( $this->getLBInfo( 'master' ) ); - # generalizeSQL will probably cut down the query to reasonable - # logging size most of the time. The substr is really just a sanity check. - if ( $isMaster ) { - $queryProf = 'query-m: ' . substr( DatabaseBase::generalizeSQL( $sql ), 0, 255 ); - $totalProf = 'DatabaseBase::query-master'; - } else { - $queryProf = 'query: ' . substr( DatabaseBase::generalizeSQL( $sql ), 0, 255 ); - $totalProf = 'DatabaseBase::query'; - } - # Include query transaction state - $queryProf .= $this->mTrxShortId ? " [TRX#{$this->mTrxShortId}]" : ""; - - $profiler = Profiler::instance(); - if ( !$profiler instanceof ProfilerStub ) { - $totalProfSection = $profiler->scopedProfileIn( $totalProf ); - $queryProfSection = $profiler->scopedProfileIn( $queryProf ); - } - if ( $this->debug() ) { wfDebugLog( 'queries', sprintf( "%s: %s", $this->mDBname, $commentedSql ) ); } @@ -852,15 +854,8 @@ abstract class DatabaseBase implements IDatabase { # Avoid fatals if close() was called $this->assertOpen(); - # Do the query and handle errors - $startTime = microtime( true ); - $ret = $this->doQuery( $commentedSql ); - $queryRuntime = microtime( true ) - $startTime; - # Log the query time and feed it into the DB trx profiler - $this->getTransactionProfiler()->recordQueryCompletion( - $queryProf, $startTime, $isWriteQuery, $this->affectedRows() ); - - MWDebug::query( $sql, $fname, $isMaster, $queryRuntime ); + # Send the query to the server + $ret = $this->doProfiledQuery( $sql, $commentedSql, $isWrite, $fname ); # Try reconnecting if the connection was lost if ( false === $ret && $this->wasErrorReissuable() ) { @@ -881,12 +876,7 @@ abstract class DatabaseBase implements IDatabase { $this->reportQueryError( $lastError, $lastErrno, $sql, $fname ); } else { # Should be safe to silently retry the query - $startTime = microtime( true ); - $ret = $this->doQuery( $commentedSql ); - $queryRuntime = microtime( true ) - $startTime; - # Log the query time and feed it into the DB trx profiler - $this->getTransactionProfiler()->recordQueryCompletion( - $queryProf, $startTime, $isWriteQuery, $this->affectedRows() ); + $ret = $this->doProfiledQuery( $sql, $commentedSql, $isWrite, $fname ); } } else { wfDebug( "Failed\n" ); @@ -911,16 +901,47 @@ abstract class DatabaseBase implements IDatabase { $res = $this->resultObject( $ret ); - // Destroy profile sections in the opposite order to their creation - ScopedCallback::consume( $queryProfSection ); - ScopedCallback::consume( $totalProfSection ); + return $res; + } - if ( $isWriteQuery && $this->mTrxLevel ) { - $this->mTrxWriteDuration += $queryRuntime; - $this->mTrxWriteCallers[] = $fname; + private function doProfiledQuery( $sql, $commentedSql, $isWrite, $fname ) { + $isMaster = !is_null( $this->getLBInfo( 'master' ) ); + # generalizeSQL() will probably cut down the query to reasonable + # logging size most of the time. The substr is really just a sanity check. + if ( $isMaster ) { + $queryProf = 'query-m: ' . substr( DatabaseBase::generalizeSQL( $sql ), 0, 255 ); + } else { + $queryProf = 'query: ' . substr( DatabaseBase::generalizeSQL( $sql ), 0, 255 ); } - return $res; + # Include query transaction state + $queryProf .= $this->mTrxShortId ? " [TRX#{$this->mTrxShortId}]" : ""; + + $profiler = Profiler::instance(); + if ( !( $profiler instanceof ProfilerStub ) ) { + $queryProfSection = $profiler->scopedProfileIn( $queryProf ); + } + + $startTime = microtime( true ); + $ret = $this->doQuery( $commentedSql ); + $queryRuntime = microtime( true ) - $startTime; + + unset( $queryProfSection ); // profile out (if set) + + if ( $ret !== false ) { + $this->lastPing = $startTime; + if ( $isWrite && $this->mTrxLevel ) { + $this->mTrxWriteDuration += $queryRuntime; + $this->mTrxWriteCallers[] = $fname; + } + } + + $this->getTransactionProfiler()->recordQueryCompletion( + $queryProf, $startTime, $isWrite, $this->affectedRows() + ); + MWDebug::query( $sql, $fname, $isMaster, $queryRuntime ); + + return $ret; } private function canRecoverFromDisconnect( $sql, $priorWritesPending ) { @@ -2926,6 +2947,9 @@ abstract class DatabaseBase implements IDatabase { } public function ping() { + if ( $this->isOpen() && ( microtime( true ) - $this->lastPing ) < self::PING_TTL ) { + return true; + } try { // This will reconnect if possible, or error out if not $this->query( "SELECT 1 AS ping", __METHOD__ ); @@ -2939,8 +2963,18 @@ abstract class DatabaseBase implements IDatabase { * @return bool */ protected function reconnect() { - # Stub. Not essential to override. - return true; + $this->closeConnection(); + $this->mOpened = false; + $this->mConn = false; + try { + $this->open( $this->mServer, $this->mUser, $this->mPassword, $this->mDBname ); + $this->lastPing = microtime( true ); + $ok = true; + } catch ( DBConnectionError $e ) { + $ok = false; + } + + return $ok; } public function getSessionLagStatus() { diff --git a/includes/db/DatabaseMysqlBase.php b/includes/db/DatabaseMysqlBase.php index d1ebe62f2e..fa3756cf2d 100644 --- a/includes/db/DatabaseMysqlBase.php +++ b/includes/db/DatabaseMysqlBase.php @@ -599,15 +599,6 @@ abstract class DatabaseMysqlBase extends Database { return strlen( $name ) && $name[0] == '`' && substr( $name, -1, 1 ) == '`'; } - function reconnect() { - $this->closeConnection(); - $this->mOpened = false; - $this->mConn = false; - $this->open( $this->mServer, $this->mUser, $this->mPassword, $this->mDBname ); - - return true; - } - function getLag() { if ( $this->getLagDetectionMethod() === 'pt-heartbeat' ) { return $this->getLagFromPtHeartbeat(); diff --git a/includes/db/loadbalancer/LoadBalancer.php b/includes/db/loadbalancer/LoadBalancer.php index f045acf93b..13a08796cb 100644 --- a/includes/db/loadbalancer/LoadBalancer.php +++ b/includes/db/loadbalancer/LoadBalancer.php @@ -1090,7 +1090,7 @@ class LoadBalancer { public function approveMasterChanges( array $options ) { $limit = isset( $options['maxWriteDuration'] ) ? $options['maxWriteDuration'] : 0; $this->forEachOpenMasterConnection( function ( DatabaseBase $conn ) use ( $limit ) { - // If atomic section or explicit transactions are still open, some caller must have + // If atomic sections or explicit transactions are still open, some caller must have // caught an exception but failed to properly rollback any changes. Detect that and // throw and error (causing rollback). if ( $conn->explicitTrxActive() ) { @@ -1108,6 +1108,14 @@ class LoadBalancer { wfMessage( 'transaction-duration-limit-exceeded', $time, $limit )->text() ); } + // If a connection sits idle while slow queries execute on another, that connection + // may end up dropped before the commit round is reached. Ping servers to detect this. + if ( $conn->writesOrCallbacksPending() && !$conn->ping() ) { + throw new DBTransactionError( + $conn, + "A connection to the {$conn->getDBname()} database was lost before commit." + ); + } } ); } diff --git a/includes/jobqueue/JobRunner.php b/includes/jobqueue/JobRunner.php index 4b906a7717..ad0abf247d 100644 --- a/includes/jobqueue/JobRunner.php +++ b/includes/jobqueue/JobRunner.php @@ -528,17 +528,6 @@ class JobRunner implements LoggerAwareInterface { $lb->waitForAll( $pos ); } - $fname = __METHOD__; - // Re-ping all masters with transactions. This throws DBError if some - // connection died while waiting on locks/slaves, triggering a rollback. - wfGetLBFactory()->forEachLB( function( LoadBalancer $lb ) use ( $fname ) { - $lb->forEachOpenConnection( function( IDatabase $conn ) use ( $fname ) { - if ( $conn->writesOrCallbacksPending() ) { - $conn->ping(); - } - } ); - } ); - // Actually commit the DB master changes wfGetLBFactory()->commitMasterChanges( __METHOD__ ); -- 2.20.1