From 8c261ee924ae27dcfca694f5e5233e81f12f5dc9 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Mon, 19 Sep 2016 16:34:32 -0700 Subject: [PATCH] Database class parameter and documentation cleanups * Document various parameter arrays. * Fix $user = false loophole in Database::__construct(). * Set the Postgres port *before* calling super, as it is needed by open(). * Remove 'chronProt' parameter as it is lazy-loaded. Change-Id: Icc1037efa1eee7ae6fdd2919f60001e6e29ae55c --- includes/libs/rdbms/database/Database.php | 82 ++++++++----- .../libs/rdbms/database/DatabasePostgres.php | 2 +- .../libs/rdbms/database/DatabaseSqlite.php | 5 +- includes/libs/rdbms/lbfactory/LBFactory.php | 24 +++- .../libs/rdbms/lbfactory/LBFactoryMulti.php | 114 +++++++++--------- .../libs/rdbms/lbfactory/LBFactorySimple.php | 12 ++ .../libs/rdbms/loadbalancer/ILoadBalancer.php | 15 ++- 7 files changed, 158 insertions(+), 96 deletions(-) diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index f9e9296b62..a5b928494d 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -231,16 +231,12 @@ abstract class Database implements IDatabase, LoggerAwareInterface { protected $trxProfiler; /** - * Constructor. - * - * FIXME: It is possible to construct a Database object with no associated - * connection object, by specifying no parameters to __construct(). This - * feature is deprecated and should be removed. + * Constructor and database handle and attempt to connect to the DB server * * IDatabase classes should not be constructed directly in external - * code. DatabaseBase::factory() should be used instead. + * code. Database::factory() should be used instead. * - * @param array $params Parameters passed from DatabaseBase::factory() + * @param array $params Parameters passed from Database::factory() */ function __construct( array $params ) { $server = $params['host']; @@ -287,37 +283,59 @@ abstract class Database implements IDatabase, LoggerAwareInterface { if ( $user ) { $this->open( $server, $user, $password, $dbName ); + } elseif ( $this->requiresDatabaseUser() ) { + throw new InvalidArgumentException( "No database user provided." ); } + // Set the domain object after open() sets the relevant fields $this->currentDomain = ( $this->mDBname != '' ) ? new DatabaseDomain( $this->mDBname, null, $this->mTablePrefix ) : DatabaseDomain::newUnspecified(); } /** - * Given a DB type, construct the name of the appropriate child class of - * IDatabase. This is designed to replace all of the manual stuff like: - * $class = 'Database' . ucfirst( strtolower( $dbType ) ); - * as well as validate against the canonical list of DB types we have - * - * This factory function is mostly useful for when you need to connect to a - * database other than the MediaWiki default (such as for external auth, - * an extension, et cetera). Do not use this to connect to the MediaWiki - * database. Example uses in core: - * @see LoadBalancer::reallyOpenConnection() - * @see ForeignDBRepo::getMasterDB() - * @see WebInstallerDBConnect::execute() - * - * @since 1.18 - * - * @param string $dbType A possible DB type - * @param array $p An array of options to pass to the constructor. - * Valid options are: host, user, password, dbname, flags, tablePrefix, schema, driver - * @return IDatabase|null If the database driver or extension cannot be found + * Construct a Database subclass instance given a database type and parameters + * + * This also connects to the database immediately upon object construction + * + * @param string $dbType A possible DB type (sqlite, mysql, postgres) + * @param array $p Parameter map with keys: + * - host : The hostname of the DB server + * - user : The name of the database user the client operates under + * - password : The password for the database user + * - dbname : The name of the database to use where queries do not specify one. + * The database must exist or an error might be thrown. Setting this to the empty string + * will avoid any such errors and make the handle have no implicit database scope. This is + * useful for queries like SHOW STATUS, CREATE DATABASE, or DROP DATABASE. Note that a + * "database" in Postgres is rougly equivalent to an entire MySQL server. This the domain + * in which user names and such are defined, e.g. users are database-specific in Postgres. + * - schema : The database schema to use (if supported). A "schema" in Postgres is roughly + * equivalent to a "database" in MySQL. Note that MySQL and SQLite do not use schemas. + * - tablePrefix : Optional table prefix that is implicitly added on to all table names + * recognized in queries. This can be used in place of schemas for handle site farms. + * - flags : Optional bitfield of DBO_* constants that define connection, protocol, + * buffering, and transaction behavior. It is STRONGLY adviced to leave the DBO_DEFAULT + * flag in place UNLESS this this database simply acts as a key/value store. + * - driver: Optional name of a specific DB client driver. For MySQL, there is the old + * 'mysql' driver and the newer 'mysqli' driver. + * - variables: Optional map of session variables to set after connecting. This can be + * used to adjust lock timeouts or encoding modes and the like. + * - connLogger: Optional PSR-3 logger interface instance. + * - queryLogger: Optional PSR-3 logger interface instance. + * - profiler: Optional class name or object with profileIn()/profileOut() methods. + * These will be called in query(), using a simplified version of the SQL that also + * includes the agent as a SQL comment. + * - trxProfiler: Optional TransactionProfiler instance. + * - errorLogger: Optional callback that takes an Exception and logs it. + * - cliMode: Whether to consider the execution context that of a CLI script. + * - agent: Optional name used to identify the end-user in query profiling/logging. + * - srvCache: Optional BagOStuff instance to an APC-style cache. + * @return Database|null If the database driver or extension cannot be found * @throws InvalidArgumentException If the database driver or extension cannot be found + * @since 1.18 */ final public static function factory( $dbType, $p = [] ) { - $canonicalDBTypes = [ + static $canonicalDBTypes = [ 'mysql' => [ 'mysqli', 'mysql' ], 'postgres' => [], 'sqlite' => [], @@ -667,7 +685,7 @@ abstract class Database implements IDatabase, LoggerAwareInterface { } /** - * Create a log context to pass to PSR logging functions. + * Create a log context to pass to PSR-3 logger functions. * * @param array $extras Additional data to add to context * @return array @@ -3456,6 +3474,14 @@ abstract class Database implements IDatabase, LoggerAwareInterface { $this->tableAliases = $aliases; } + /** + * @return bool Whether a DB user is required to access the DB + * @since 1.28 + */ + protected function requiresDatabaseUser() { + return true; + } + /** * @since 1.19 * @return string diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index b07ac164e2..e79b28bf6b 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -43,8 +43,8 @@ class DatabasePostgres extends DatabaseBase { private $mCoreSchema; public function __construct( array $params ) { - parent::__construct( $params ); $this->port = isset( $params['port'] ) ? $params['port'] : false; + parent::__construct( $params ); } function getType() { diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php index 888198365b..6614898fef 100644 --- a/includes/libs/rdbms/database/DatabaseSqlite.php +++ b/includes/libs/rdbms/database/DatabaseSqlite.php @@ -1046,11 +1046,14 @@ class DatabaseSqlite extends DatabaseBase { return $this->query( $sql, $fName ); } + protected function requiresDatabaseUser() { + return false; // just a file + } + /** * @return string */ public function __toString() { return 'SQLite ' . (string)$this->mConn->getAttribute( PDO::ATTR_SERVER_VERSION ); } - } diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index 40ba458826..38132c74ba 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -80,8 +80,26 @@ abstract class LBFactory { [ 'replLogger', 'connLogger', 'queryLogger', 'perfLogger' ]; /** - * @TODO: document base params here - * @param array $conf + * Construct a manager of ILoadBalancer objects + * + * Sub-classes will extend the required keys in $conf with additional parameters + * + * @param $conf $params Array with keys: + * - localDomain: A DatabaseDomain or domain ID string. + * - readOnlyReason : Reason the master DB is read-only if so [optional] + * - srvCache : BagOStuff object for server cache [optional] + * - memCache : BagOStuff object for cluster memory cache [optional] + * - wanCache : WANObjectCache object [optional] + * - hostname : The name of the current server [optional] + * - cliMode: Whether the execution context is a CLI script. [optional] + * - profiler : Class name or instance with profileIn()/profileOut() methods. [optional] + * - trxProfiler: TransactionProfiler instance. [optional] + * - replLogger: PSR-3 logger instance. [optional] + * - connLogger: PSR-3 logger instance. [optional] + * - queryLogger: PSR-3 logger instance. [optional] + * - perfLogger: PSR-3 logger instance. [optional] + * - errorLogger : Callback that takes an Exception and logs it. [optional] + * @throws InvalidArgumentException */ public function __construct( array $conf ) { $this->localDomain = isset( $conf['localDomain'] ) @@ -107,8 +125,6 @@ abstract class LBFactory { trigger_error( E_WARNING, get_class( $e ) . ': ' . $e->getMessage() ); }; - $this->chronProt = isset( $conf['chronProt'] ) ? $conf['chronProt'] : null; - $this->profiler = isset( $params['profiler'] ) ? $params['profiler'] : null; $this->trxProfiler = isset( $conf['trxProfiler'] ) ? $conf['trxProfiler'] diff --git a/includes/libs/rdbms/lbfactory/LBFactoryMulti.php b/includes/libs/rdbms/lbfactory/LBFactoryMulti.php index 0f1493a82b..25e1fe022e 100644 --- a/includes/libs/rdbms/lbfactory/LBFactoryMulti.php +++ b/includes/libs/rdbms/lbfactory/LBFactoryMulti.php @@ -25,62 +25,6 @@ * A multi-database, multi-master factory for Wikimedia and similar installations. * Ignores the old configuration globals. * - * Template override precedence (highest => lowest): - * - templateOverridesByServer - * - masterTemplateOverrides - * - templateOverridesBySection/templateOverridesByCluster - * - externalTemplateOverrides - * - serverTemplate - * Overrides only work on top level keys (so nested values will not be merged). - * - * Configuration: - * sectionsByDB A map of database names to section names. - * - * sectionLoads A 2-d map. For each section, gives a map of server names to - * load ratios. For example: - * [ - * 'section1' => [ - * 'db1' => 100, - * 'db2' => 100 - * ] - * ] - * - * serverTemplate A server info associative array as documented for $wgDBservers. - * The host, hostName and load entries will be overridden. - * - * groupLoadsBySection A 3-d map giving server load ratios for each section and group. - * For example: - * [ - * 'section1' => [ - * 'group1' => [ - * 'db1' => 100, - * 'db2' => 100 - * ] - * ] - * ] - * - * groupLoadsByDB A 3-d map giving server load ratios by DB name. - * - * hostsByName A map of hostname to IP address. - * - * externalLoads A map of external storage cluster name to server load map. - * - * externalTemplateOverrides A set of server info keys overriding serverTemplate for external - * storage. - * - * templateOverridesByServer A 2-d map overriding serverTemplate and - * externalTemplateOverrides on a server-by-server basis. Applies - * to both core and external storage. - * templateOverridesBySection A 2-d map overriding the server info by section. - * templateOverridesByCluster A 2-d map overriding the server info by external storage cluster. - * - * masterTemplateOverrides An override array for all master servers. - * - * loadMonitorClass Name of the LoadMonitor class to always use. - * - * readOnlyBySection A map of section name to read-only message. - * Missing or false for read/write. - * * @ingroup Database */ class LBFactoryMulti extends LBFactory { @@ -141,8 +85,6 @@ class LBFactoryMulti extends LBFactory { */ private $readOnlyBySection = []; - // Other stuff - /** @var array Load balancer factory configuration */ private $conf; @@ -162,8 +104,60 @@ class LBFactoryMulti extends LBFactory { private $lastSection; /** - * @param array $conf - * @throws InvalidArgumentException + * @see LBFactory::__construct() + * + * Template override precedence (highest => lowest): + * - templateOverridesByServer + * - masterTemplateOverrides + * - templateOverridesBySection/templateOverridesByCluster + * - externalTemplateOverrides + * - serverTemplate + * Overrides only work on top level keys (so nested values will not be merged). + * + * Server configuration maps should be of the format Database::factory() requires. + * Additionally, a 'max lag' key should also be set on server maps, indicating how stale the + * data can be before the load balancer tries to avoid using it. The map can have 'is static' + * set to disable blocking replication sync checks (intended for archive servers with + * unchanging data). + * + * @param array $conf Parameters of LBFactory::__construct() as well as: + * - sectionsByDB Map of database names to section names. + * - sectionLoads 2-d map. For each section, gives a map of server names to + * load ratios. For example: + * [ + * 'section1' => [ + * 'db1' => 100, + * 'db2' => 100 + * ] + * ] + * - serverTemplate Server configuration map intended for Database::factory(). + * Note that "host", "hostName" and "load" entries will be + * overridden by "sectionLoads" and "hostsByName". + * - groupLoadsBySection 3-d map giving server load ratios for each section/group. + * For example: + * [ + * 'section1' => [ + * 'group1' => [ + * 'db1' => 100, + * 'db2' => 100 + * ] + * ] + * ] + * - groupLoadsByDB 3-d map giving server load ratios by DB name. + * - hostsByName Map of hostname to IP address. + * - externalLoads Map of external storage cluster name to server load map. + * - externalTemplateOverrides Set of server configuration maps overriding + * "serverTemplate" for external storage. + * - templateOverridesByServer 2-d map overriding "serverTemplate" and + * "externalTemplateOverrides" on a server-by-server basis. + * Applies to both core and external storage. + * - templateOverridesBySection 2-d map overriding the server configuration maps by section. + * - templateOverridesByCluster 2-d map overriding the server configuration maps by external + * storage cluster. + * - masterTemplateOverrides Server configuration map overrides for all master servers. + * - loadMonitorClass Name of the LoadMonitor class to always use. + * - readOnlyBySection A map of section name to read-only message. + * Missing or false for read/write. */ public function __construct( array $conf ) { parent::__construct( $conf ); diff --git a/includes/libs/rdbms/lbfactory/LBFactorySimple.php b/includes/libs/rdbms/lbfactory/LBFactorySimple.php index 0949092546..b90afe603c 100644 --- a/includes/libs/rdbms/lbfactory/LBFactorySimple.php +++ b/includes/libs/rdbms/lbfactory/LBFactorySimple.php @@ -38,6 +38,18 @@ class LBFactorySimple extends LBFactory { /** @var string */ private $loadMonitorClass; + /** + * @see LBFactory::__construct() + * @param array $conf Parameters of LBFactory::__construct() as well as: + * - servers : list of server configuration maps to Database::factory(). + * Additionally, the server maps should have a 'load' key, which is used to decide + * how often clients connect to one server verses the others. A 'max lag' key should + * also be set on server maps, indicating how stale the data can be before the load + * balancer tries to avoid using it. The map can have 'is static' set to disable blocking + * replication sync checks (intended for archive servers with unchanging data). + * - externalClusters : map of cluster names to server arrays. The servers arrays have the + * same format as "servers" above. + */ public function __construct( array $conf ) { parent::__construct( $conf ); diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php index 0f6bea3f93..cdbfc77503 100644 --- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php @@ -30,15 +30,26 @@ */ interface ILoadBalancer { /** - * @param array $params Array with keys: + * Construct a manager of IDatabase connection objects + * + * @param array $params Parameter map with keys: * - servers : Required. Array of server info structures. + * - localDomain: A DatabaseDomain or domain ID string. * - loadMonitor : Name of a class used to fetch server lag and load. * - readOnlyReason : Reason the master DB is read-only if so [optional] * - waitTimeout : Maximum time to wait for replicas for consistency [optional] * - srvCache : BagOStuff object for server cache [optional] * - memCache : BagOStuff object for cluster memory cache [optional] * - wanCache : WANObjectCache object [optional] - * - hostname : the name of the current server [optional] + * - hostname : The name of the current server [optional] + * - cliMode: Whether the execution context is a CLI script. [optional] + * - profiler : Class name or instance with profileIn()/profileOut() methods. [optional] + * - trxProfiler: TransactionProfiler instance. [optional] + * - replLogger: PSR-3 logger instance. [optional] + * - connLogger: PSR-3 logger instance. [optional] + * - queryLogger: PSR-3 logger instance. [optional] + * - perfLogger: PSR-3 logger instance. [optional] + * - errorLogger : Callback that takes an Exception and logs it. [optional] * @throws InvalidArgumentException */ public function __construct( array $params ); -- 2.20.1