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

diff --git a/includes/db/Database.php b/includes/db/Database.php
index 186a87b..6ddc9f7 100644 (file)
--- 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 d1ebe62..fa3756c 100644 (file)
--- 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 f045acf..13a0879 100644 (file)
--- 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 4b906a7..ad0abf2 100644 (file)
--- 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__ );