From 16b4e3a9f17df4c56e8d49692ef01ac367b7ae10 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 15 Sep 2016 02:21:21 -0700 Subject: [PATCH] Avoid global state in DatabaseBase::factory()/query() Change-Id: Ibb4f1c0dafea071a1c34e0cd5b5c15b8b4bb7bc6 --- includes/MediaWiki.php | 10 +++++++ includes/db/Database.php | 29 +++++++------------ includes/db/loadbalancer/LBFactoryMW.php | 5 ++++ includes/libs/rdbms/lbfactory/LBFactory.php | 24 +++++++++++++-- .../libs/rdbms/loadbalancer/LoadBalancer.php | 11 ++++++- maintenance/Maintenance.php | 13 ++++++++- maintenance/doMaintenance.php | 2 +- 7 files changed, 69 insertions(+), 25 deletions(-) diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php index 9bbbd352b4..2aa4b80c66 100644 --- a/includes/MediaWiki.php +++ b/includes/MediaWiki.php @@ -517,6 +517,7 @@ class MediaWiki { */ public function run() { try { + $this->setDBProfilingAgent(); try { $this->main(); } catch ( ErrorPageError $e ) { @@ -533,6 +534,15 @@ class MediaWiki { $this->doPostOutputShutdown( 'normal' ); } + private function setDBProfilingAgent() { + $services = MediaWikiServices::getInstance(); + // Add a comment for easy SHOW PROCESSLIST interpretation + $name = $this->context->getUser()->getName(); + $services->getDBLoadBalancerFactory()->setAgentName( + mb_strlen( $name ) > 15 ? mb_substr( $name, 0, 15 ) . '...' : $name + ); + } + /** * @see MediaWiki::preOutputCommit() * @param callable $postCommitWork [default: null] diff --git a/includes/db/Database.php b/includes/db/Database.php index 3fa1335e2c..232bd38208 100644 --- a/includes/db/Database.php +++ b/includes/db/Database.php @@ -62,8 +62,10 @@ abstract class DatabaseBase implements IDatabase, LoggerAwareInterface { protected $mDBname; /** @var array[] $aliases Map of (table => (dbname, schema, prefix) map) */ protected $tableAliases = []; - /** @var bool */ + /** @var bool Whether this PHP instance is for a CLI script */ protected $cliMode; + /** @var string Agent name for query profiling */ + protected $agent; /** @var BagOStuff APC cache */ protected $srvCache; @@ -251,6 +253,9 @@ abstract class DatabaseBase implements IDatabase, LoggerAwareInterface { $this->cliMode = isset( $params['cliMode'] ) ? $params['cliMode'] : ( PHP_SAPI === 'cli' ); + $this->agent = isset( $params['agent'] ) + ? str_replace( '/', '-', $params['agent'] ) // escape for comment + : ''; $this->mFlags = $flags; if ( $this->mFlags & DBO_DEFAULT ) { @@ -308,8 +313,6 @@ abstract class DatabaseBase implements IDatabase, LoggerAwareInterface { * @throws InvalidArgumentException If the database driver or extension cannot be found */ final public static function factory( $dbType, $p = [] ) { - global $wgCommandLineMode; - $canonicalDBTypes = [ 'mysql' => [ 'mysqli', 'mysql' ], 'postgres' => [], @@ -367,7 +370,6 @@ abstract class DatabaseBase implements IDatabase, LoggerAwareInterface { $p['schema'] = isset( $defaultSchemas[$dbType] ) ? $defaultSchemas[$dbType] : null; } $p['foreign'] = isset( $p['foreign'] ) ? $p['foreign'] : false; - $p['cliMode'] = $wgCommandLineMode; $conn = new $class( $p ); if ( isset( $p['connLogger'] ) ) { @@ -379,7 +381,9 @@ abstract class DatabaseBase implements IDatabase, LoggerAwareInterface { if ( isset( $p['errorLogger'] ) ) { $conn->errorLogger = $p['errorLogger']; } else { - $conn->errorLogger = [ MWExceptionHandler::class, 'logException' ]; + $conn->errorLogger = function ( Exception $e ) { + trigger_error( get_class( $e ) . ': ' . $e->getMessage(), E_WARNING ); + }; } } else { $conn = null; @@ -831,8 +835,6 @@ abstract class DatabaseBase implements IDatabase, LoggerAwareInterface { } public function query( $sql, $fname = __METHOD__, $tempIgnore = false ) { - global $wgUser; - $priorWritesPending = $this->writesOrCallbacksPending(); $this->mLastQuery = $sql; @@ -846,20 +848,9 @@ abstract class DatabaseBase implements IDatabase, LoggerAwareInterface { $this->mDoneWrites = microtime( true ); } - # Add a comment for easy SHOW PROCESSLIST interpretation - if ( is_object( $wgUser ) && $wgUser->isItemLoaded( 'name' ) ) { - $userName = $wgUser->getName(); - if ( mb_strlen( $userName ) > 15 ) { - $userName = mb_substr( $userName, 0, 15 ) . '...'; - } - $userName = str_replace( '/', '', $userName ); - } else { - $userName = ''; - } - // Add trace comment to the begin of the sql string, right after the operator. // Or, for one-word queries (like "BEGIN" or COMMIT") add it to the end (bug 42598) - $commentedSql = preg_replace( '/\s|$/', " /* $fname $userName */ ", $sql, 1 ); + $commentedSql = preg_replace( '/\s|$/', " /* $fname {$this->agent} */ ", $sql, 1 ); # Start implicit transactions that wrap the request if DBO_TRX is enabled if ( !$this->mTrxLevel && $this->getFlag( DBO_TRX ) diff --git a/includes/db/loadbalancer/LBFactoryMW.php b/includes/db/loadbalancer/LBFactoryMW.php index 87fd81bb6c..33c48a5384 100644 --- a/includes/db/loadbalancer/LBFactoryMW.php +++ b/includes/db/loadbalancer/LBFactoryMW.php @@ -37,6 +37,8 @@ abstract class LBFactoryMW extends LBFactory implements DestructibleService { * @TODO: inject objects via dependency framework */ public function __construct( array $conf ) { + global $wgCommandLineMode; + $defaults = [ 'domain' => wfWikiID(), 'hostname' => wfHostname(), @@ -61,6 +63,9 @@ abstract class LBFactoryMW extends LBFactory implements DestructibleService { $defaults['wanCache'] = $wCache; } + $this->agent = isset( $params['agent'] ) ? $params['agent'] : ''; + $this->cliMode = isset( $params['cliMode'] ) ? $params['cliMode'] : $wgCommandLineMode; + parent::__construct( $conf + $defaults ); } diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index feae4bd05b..107a7e2ddf 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -62,6 +62,11 @@ abstract class LBFactory { /** @var callable[] */ protected $replicationWaitCallbacks = []; + /** @var bool Whether this PHP instance is for a CLI script */ + protected $cliMode; + /** @var string Agent name for query profiling */ + protected $agent; + const SHUTDOWN_NO_CHRONPROT = 0; // don't save DB positions at all const SHUTDOWN_CHRONPROT_ASYNC = 1; // save DB positions, but don't wait on remote DCs const SHUTDOWN_CHRONPROT_SYNC = 2; // save DB positions, waiting on all DCs @@ -75,6 +80,7 @@ abstract class LBFactory { */ public function __construct( array $conf ) { $this->domain = isset( $conf['domain'] ) ? $conf['domain'] : ''; + if ( isset( $conf['readOnlyReason'] ) && is_string( $conf['readOnlyReason'] ) ) { $this->readOnlyReason = $conf['readOnlyReason']; } @@ -91,7 +97,7 @@ abstract class LBFactory { $this->errorLogger = isset( $conf['errorLogger'] ) ? $conf['errorLogger'] : function ( Exception $e ) { - trigger_error( E_WARNING, $e->getMessage() ); + trigger_error( E_WARNING, get_class( $e ) . ': ' . $e->getMessage() ); }; $this->hostname = isset( $conf['hostname'] ) ? $conf['hostname'] @@ -105,6 +111,8 @@ abstract class LBFactory { : new TransactionProfiler(); $this->ticket = mt_rand(); + $this->cliMode = isset( $params['cliMode'] ) ? $params['cliMode'] : PHP_SAPI === 'cli'; + $this->agent = isset( $params['agent'] ) ? $params['agent'] : ''; } /** @@ -554,7 +562,7 @@ abstract class LBFactory { isset( $_GET['cpPosTime'] ) ? $_GET['cpPosTime'] : null ); $chronProt->setLogger( $this->replLogger ); - if ( PHP_SAPI === 'cli' ) { + if ( $this->cliMode ) { $chronProt->setEnabled( false ); } @@ -609,7 +617,9 @@ abstract class LBFactory { 'connLogger' => $this->connLogger, 'replLogger' => $this->replLogger, 'errorLogger' => $this->errorLogger, - 'hostname' => $this->hostname + 'hostname' => $this->hostname, + 'cliMode' => $this->cliMode, + 'agent' => $this->agent ]; } @@ -641,4 +651,12 @@ abstract class LBFactory { public function closeAll() { $this->forEachLBCallMethod( 'closeAll', [] ); } + + /** + * @param string $agent Agent name for query profiling + * @since 1.28 + */ + public function setAgentName( $agent ) { + $this->agent = $agent; + } } diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index cea7523ba5..628e452f45 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -88,6 +88,10 @@ class LoadBalancer implements ILoadBalancer { private $localDomain; /** @var string Current server name */ private $host; + /** @var bool Whether this PHP instance is for a CLI script */ + protected $cliMode; + /** @var string Agent name for query profiling */ + protected $agent; /** @var callable Exception logger */ private $errorLogger; @@ -175,7 +179,7 @@ class LoadBalancer implements ILoadBalancer { $this->errorLogger = isset( $params['errorLogger'] ) ? $params['errorLogger'] : function ( Exception $e ) { - trigger_error( E_WARNING, $e->getMessage() ); + trigger_error( get_class( $e ) . ': ' . $e->getMessage(), E_WARNING ); }; foreach ( [ 'replLogger', 'connLogger', 'queryLogger', 'perfLogger' ] as $key ) { @@ -185,6 +189,8 @@ class LoadBalancer implements ILoadBalancer { $this->host = isset( $params['hostname'] ) ? $params['hostname'] : ( gethostname() ?: 'unknown' ); + $this->cliMode = isset( $params['cliMode'] ) ? $params['cliMode'] : PHP_SAPI === 'cli'; + $this->agent = isset( $params['agent'] ) ? $params['agent'] : ''; } /** @@ -810,6 +816,9 @@ class LoadBalancer implements ILoadBalancer { $server['connLogger'] = $this->connLogger; $server['queryLogger'] = $this->queryLogger; $server['trxProfiler'] = $this->trxProfiler; + $server['cliMode'] = $this->cliMode; + $server['errorLogger'] = $this->errorLogger; + $server['agent'] = $this->agent; // Create a live connection object try { diff --git a/maintenance/Maintenance.php b/maintenance/Maintenance.php index 1fbca1417c..7e0fb455bf 100644 --- a/maintenance/Maintenance.php +++ b/maintenance/Maintenance.php @@ -553,8 +553,19 @@ abstract class Maintenance { * Set triggers like when to try to run deferred updates * @since 1.28 */ - public function setTriggers() { + public function setAgentAndTriggers() { + if ( function_exists( 'posix_getpwuid' ) ) { + $agent = posix_getpwuid( posix_geteuid() )['name']; + } else { + $agent = 'sysadmin'; + } + $agent .= '@' . wfHostname(); + $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory(); + // Add a comment for easy SHOW PROCESSLIST interpretation + $lbFactory->setAgentName( + mb_strlen( $agent ) > 15 ? mb_substr( $agent, 0, 15 ) . '...' : $agent + ); self::setLBFactoryTriggers( $lbFactory ); } diff --git a/maintenance/doMaintenance.php b/maintenance/doMaintenance.php index 890fe45a63..60b24a2db0 100644 --- a/maintenance/doMaintenance.php +++ b/maintenance/doMaintenance.php @@ -104,7 +104,7 @@ $maintenance->checkRequiredExtensions(); // A good time when no DBs have writes pending is around lag checks. // This avoids having long running scripts just OOM and lose all the updates. -$maintenance->setTriggers(); +$maintenance->setAgentAndTriggers(); // Do the work $maintenance->execute(); -- 2.20.1