From fe59c39da94feb9cc02c593327eda91de470e44d Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Mon, 28 Nov 2016 10:26:14 -0800 Subject: [PATCH] Add LoadBalancer::getMaintenanceConnectionRef() method This is useful when IMaintainableDatabase methods are needed for foreign wiki connections to things like external store. Also: * Set visibility for ExternalStoreDB methods. * Cleaned up various type hints and comments. Change-Id: Ie35b1ff21032cc4e78912dc499486da23aeba041 --- autoload.php | 1 + includes/db/CloneDatabase.php | 9 +-- includes/externalstore/ExternalStoreDB.php | 19 +++--- includes/libs/rdbms/database/DBConnRef.php | 12 ++-- includes/libs/rdbms/database/Database.php | 19 +----- .../rdbms/database/IMaintainableDatabase.php | 19 ++++++ .../rdbms/database/MaintainableDBConnRef.php | 68 +++++++++++++++++++ .../libs/rdbms/loadbalancer/ILoadBalancer.php | 61 +++++++++++------ .../libs/rdbms/loadbalancer/LoadBalancer.php | 6 ++ tests/phpunit/MediaWikiTestCase.php | 14 ++-- 10 files changed, 161 insertions(+), 67 deletions(-) create mode 100644 includes/libs/rdbms/database/MaintainableDBConnRef.php diff --git a/autoload.php b/autoload.php index e079686bc4..66a9e9b78d 100644 --- a/autoload.php +++ b/autoload.php @@ -803,6 +803,7 @@ $wgAutoloadLocalClasses = [ 'MagicWordArray' => __DIR__ . '/includes/MagicWordArray.php', 'MailAddress' => __DIR__ . '/includes/mail/MailAddress.php', 'MainConfigDependency' => __DIR__ . '/includes/cache/CacheDependency.php', + 'MaintainableDBConnRef' => __DIR__ . '/includes/libs/rdbms/database/MaintainableDBConnRef.php', 'Maintenance' => __DIR__ . '/maintenance/Maintenance.php', 'MaintenanceFormatInstallDoc' => __DIR__ . '/maintenance/formatInstallDoc.php', 'MakeTestEdits' => __DIR__ . '/maintenance/makeTestEdits.php', diff --git a/includes/db/CloneDatabase.php b/includes/db/CloneDatabase.php index f1ccd2aebd..2b394b6d7b 100644 --- a/includes/db/CloneDatabase.php +++ b/includes/db/CloneDatabase.php @@ -41,19 +41,19 @@ class CloneDatabase { /** @var bool Whether to use temporary tables or not */ private $useTemporaryTables = true; - /** @var Database */ + /** @var IMaintainableDatabase */ private $db; /** * Constructor * - * @param Database $db A database subclass + * @param IMaintainableDatabase $db A database subclass * @param array $tablesToClone An array of tables to clone, unprefixed * @param string $newTablePrefix Prefix to assign to the tables * @param string $oldTablePrefix Prefix on current tables, if not $wgDBprefix * @param bool $dropCurrentTables */ - public function __construct( Database $db, array $tablesToClone, + public function __construct( IMaintainableDatabase $db, array $tablesToClone, $newTablePrefix, $oldTablePrefix = '', $dropCurrentTables = true ) { $this->db = $db; @@ -107,7 +107,8 @@ class CloneDatabase { # Create new table wfDebug( __METHOD__ . " duplicating $oldTableName to $newTableName\n" ); - $this->db->duplicateTableStructure( $oldTableName, $newTableName, $this->useTemporaryTables ); + $this->db->duplicateTableStructure( + $oldTableName, $newTableName, $this->useTemporaryTables ); } } diff --git a/includes/externalstore/ExternalStoreDB.php b/includes/externalstore/ExternalStoreDB.php index 7e932994e6..52c1a4c1f1 100644 --- a/includes/externalstore/ExternalStoreDB.php +++ b/includes/externalstore/ExternalStoreDB.php @@ -105,7 +105,7 @@ class ExternalStoreDB extends ExternalStoreMedium { * @param string $cluster Cluster name * @return LoadBalancer */ - function getLoadBalancer( $cluster ) { + private function getLoadBalancer( $cluster ) { $wiki = isset( $this->params['wiki'] ) ? $this->params['wiki'] : false; return wfGetLBFactory()->getExternalLB( $cluster, $wiki ); @@ -115,9 +115,9 @@ class ExternalStoreDB extends ExternalStoreMedium { * Get a replica DB connection for the specified cluster * * @param string $cluster Cluster name - * @return IDatabase + * @return DBConnRef */ - function getSlave( $cluster ) { + public function getSlave( $cluster ) { global $wgDefaultExternalStore; $wiki = isset( $this->params['wiki'] ) ? $this->params['wiki'] : false; @@ -140,13 +140,13 @@ class ExternalStoreDB extends ExternalStoreMedium { * Get a master database connection for the specified cluster * * @param string $cluster Cluster name - * @return IDatabase + * @return MaintainableDBConnRef */ - function getMaster( $cluster ) { + public function getMaster( $cluster ) { $wiki = isset( $this->params['wiki'] ) ? $this->params['wiki'] : false; $lb = $this->getLoadBalancer( $cluster ); - $db = $lb->getConnectionRef( DB_MASTER, [], $wiki ); + $db = $lb->getMaintenanceConnectionRef( DB_MASTER, [], $wiki ); $db->clearFlag( DBO_TRX ); // sanity return $db; @@ -158,7 +158,7 @@ class ExternalStoreDB extends ExternalStoreMedium { * @param IDatabase $db * @return string Table name ('blobs' by default) */ - function getTable( $db ) { + public function getTable( $db ) { $table = $db->getLBInfo( 'blobs table' ); if ( is_null( $table ) ) { $table = 'blobs'; @@ -175,9 +175,8 @@ class ExternalStoreDB extends ExternalStoreMedium { * @param string $id * @param string $itemID * @return HistoryBlob|bool Returns false if missing - * @private */ - function fetchBlob( $cluster, $id, $itemID ) { + private function fetchBlob( $cluster, $id, $itemID ) { /** * One-step cache variable to hold base blobs; operations that * pull multiple revisions may often pull multiple times from @@ -230,7 +229,7 @@ class ExternalStoreDB extends ExternalStoreMedium { * @return array A map from the blob_id's requested to their content. * Unlocated ids are not represented */ - function batchFetchBlobs( $cluster, array $ids ) { + private function batchFetchBlobs( $cluster, array $ids ) { $dbr = $this->getSlave( $cluster ); $res = $dbr->select( $this->getTable( $dbr ), [ 'blob_id', 'blob_text' ], [ 'blob_id' => array_keys( $ids ) ], __METHOD__ ); diff --git a/includes/libs/rdbms/database/DBConnRef.php b/includes/libs/rdbms/database/DBConnRef.php index 20198bf14c..b268b9f28c 100644 --- a/includes/libs/rdbms/database/DBConnRef.php +++ b/includes/libs/rdbms/database/DBConnRef.php @@ -10,10 +10,8 @@ class DBConnRef implements IDatabase { /** @var ILoadBalancer */ private $lb; - - /** @var IDatabase|null Live connection handle */ + /** @var Database|null Live connection handle */ private $conn; - /** @var array|null N-tuple of (server index, group, DatabaseDomain|string) */ private $params; @@ -22,12 +20,12 @@ class DBConnRef implements IDatabase { const FLD_DOMAIN = 2; /** - * @param ILoadBalancer $lb - * @param IDatabase|array $conn Connection or (server index, group, DatabaseDomain|string) + * @param ILoadBalancer $lb Connection manager for $conn + * @param Database|array $conn New connection handle or (server index, query groups, domain) */ public function __construct( ILoadBalancer $lb, $conn ) { $this->lb = $lb; - if ( $conn instanceof IDatabase ) { + if ( $conn instanceof Database ) { $this->conn = $conn; // live handle } elseif ( count( $conn ) >= 3 && $conn[self::FLD_DOMAIN] !== false ) { $this->params = $conn; @@ -595,7 +593,7 @@ class DBConnRef implements IDatabase { * Clean up the connection when out of scope */ function __destruct() { - if ( $this->conn !== null ) { + if ( $this->conn ) { $this->lb->reuseConnection( $this->conn ); } } diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 3d35d76ca3..b9beac8662 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -2862,23 +2862,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware return $this->mTrxLevel && ( $this->mTrxAtomicLevels || !$this->mTrxAutomatic ); } - /** - * Creates a new table with structure copied from existing table - * Note that unlike most database abstraction functions, this function does not - * automatically append database prefix, because it works at a lower - * abstraction level. - * The table names passed to this function shall not be quoted (this - * function calls addIdentifierQuotes when needed). - * - * @param string $oldName Name of table whose structure should be copied - * @param string $newName Name of table to be created - * @param bool $temporary Whether the new table should be temporary - * @param string $fname Calling function name - * @throws RuntimeException - * @return bool True if operation was successful - */ - public function duplicateTableStructure( $oldName, $newName, $temporary = false, - $fname = __METHOD__ + public function duplicateTableStructure( + $oldName, $newName, $temporary = false, $fname = __METHOD__ ) { throw new RuntimeException( __METHOD__ . ' is not implemented in descendant class' ); } diff --git a/includes/libs/rdbms/database/IMaintainableDatabase.php b/includes/libs/rdbms/database/IMaintainableDatabase.php index 83953596f1..43cec28a62 100644 --- a/includes/libs/rdbms/database/IMaintainableDatabase.php +++ b/includes/libs/rdbms/database/IMaintainableDatabase.php @@ -186,4 +186,23 @@ interface IMaintainableDatabase extends IDatabase { * @return array */ public function listViews( $prefix = null, $fname = __METHOD__ ); + + /** + * Creates a new table with structure copied from existing table + * + * Note that unlike most database abstraction functions, this function does not + * automatically append database prefix, because it works at a lower abstraction level. + * The table names passed to this function shall not be quoted (this function calls + * addIdentifierQuotes() when needed). + * + * @param string $oldName Name of table whose structure should be copied + * @param string $newName Name of table to be created + * @param bool $temporary Whether the new table should be temporary + * @param string $fname Calling function name + * @return bool True if operation was successful + * @throws RuntimeException + */ + public function duplicateTableStructure( + $oldName, $newName, $temporary = false, $fname = __METHOD__ + ); } diff --git a/includes/libs/rdbms/database/MaintainableDBConnRef.php b/includes/libs/rdbms/database/MaintainableDBConnRef.php new file mode 100644 index 0000000000..fa3ddf9eb9 --- /dev/null +++ b/includes/libs/rdbms/database/MaintainableDBConnRef.php @@ -0,0 +1,68 @@ +__call( __FUNCTION__, func_get_args() ); + } + + public function tableNames() { + return $this->__call( __FUNCTION__, func_get_args() ); + } + + public function tableNamesN() { + return $this->__call( __FUNCTION__, func_get_args() ); + } + + public function sourceFile( + $filename, + callable $lineCallback = null, + callable $resultCallback = null, + $fname = false, + callable $inputCallback = null + ) { + return $this->__call( __FUNCTION__, func_get_args() ); + } + + public function sourceStream( + $fp, + callable $lineCallback = null, + callable $resultCallback = null, + $fname = __METHOD__, + callable $inputCallback = null + ) { + return $this->__call( __FUNCTION__, func_get_args() ); + } + + public function dropTable( $tableName, $fName = __METHOD__ ) { + return $this->__call( __FUNCTION__, func_get_args() ); + } + + public function deadlockLoop() { + return $this->__call( __FUNCTION__, func_get_args() ); + } + + public function listViews( $prefix = null, $fname = __METHOD__ ) { + return $this->__call( __FUNCTION__, func_get_args() ); + } + + public function textFieldSize( $table, $field ) { + return $this->__call( __FUNCTION__, func_get_args() ); + } + + public function streamStatementEnd( &$sql, &$newLine ) { + return $this->__call( __FUNCTION__, func_get_args() ); + } + + public function duplicateTableStructure( + $oldName, $newName, $temporary = false, $fname = __METHOD__ + ) { + return $this->__call( __FUNCTION__, func_get_args() ); + } +} diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php index 8854479a63..fc306b49b4 100644 --- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php @@ -108,8 +108,9 @@ interface ILoadBalancer { /** * Get the index of the reader connection, which may be a replica DB + * * This takes into account load ratios and lag times. It should - * always return a consistent index during a given invocation + * always return a consistent index during a given invocation. * * Side effect: opens connections to databases * @param string|bool $group Query group, or false for the generic reader @@ -121,8 +122,10 @@ interface ILoadBalancer { /** * Set the master wait position - * If a DB_REPLICA connection has been opened already, waits - * Otherwise sets a variable telling it to wait if such a connection is opened + * + * If a DB_REPLICA connection has been opened already, then wait immediately. + * Otherwise sets a variable telling it to wait if such a connection is opened. + * * @param DBMasterPos $pos */ public function waitFor( $pos ); @@ -140,6 +143,7 @@ interface ILoadBalancer { /** * Set the master wait position and wait for ALL replica DBs to catch up to it + * * @param DBMasterPos $pos * @param int $timeout Max seconds to wait; default is mWaitTimeout * @return bool Success (able to connect and no timeouts reached) @@ -148,30 +152,29 @@ interface ILoadBalancer { /** * Get any open connection to a given server index, local or foreign - * Returns false if there is no connection open * - * @param int $i Server index - * @return IDatabase|bool False on failure + * @param int $i Server index or DB_MASTER/DB_REPLICA + * @return Database|bool False if no such connection is open */ public function getAnyOpenConnection( $i ); /** * Get a connection by index - * This is the main entry point for this class. * - * @param int $i Server index + * @param int $i Server index or DB_MASTER/DB_REPLICA * @param array|string|bool $groups Query group(s), or false for the generic reader * @param string|bool $domain Domain ID, or false for the current domain * * @throws DBError - * @return IDatabase + * @return Database */ public function getConnection( $i, $groups = [], $domain = false ); /** - * Mark a foreign connection as being available for reuse under a different - * DB name or prefix. This mechanism is reference-counted, and must be called - * the same number of times as getConnection() to work. + * Mark a foreign connection as being available for reuse under a different DB domain + * + * This mechanism is reference-counted, and must be called the same number of times + * as getConnection() to work. * * @param IDatabase $conn * @throws InvalidArgumentException @@ -181,30 +184,44 @@ interface ILoadBalancer { /** * Get a database connection handle reference * - * The handle's methods wrap simply wrap those of a IDatabase handle + * The handle's methods simply wrap those of a Database handle * - * @see LoadBalancer::getConnection() for parameter information + * @see ILoadBalancer::getConnection() for parameter information * - * @param int $db + * @param int $i Server index or DB_MASTER/DB_REPLICA * @param array|string|bool $groups Query group(s), or false for the generic reader * @param string|bool $domain Domain ID, or false for the current domain * @return DBConnRef */ - public function getConnectionRef( $db, $groups = [], $domain = false ); + public function getConnectionRef( $i, $groups = [], $domain = false ); /** * Get a database connection handle reference without connecting yet * - * The handle's methods wrap simply wrap those of a IDatabase handle + * The handle's methods simply wrap those of a Database handle * - * @see LoadBalancer::getConnection() for parameter information + * @see ILoadBalancer::getConnection() for parameter information * - * @param int $db + * @param int $i Server index or DB_MASTER/DB_REPLICA * @param array|string|bool $groups Query group(s), or false for the generic reader * @param string|bool $domain Domain ID, or false for the current domain * @return DBConnRef */ - public function getLazyConnectionRef( $db, $groups = [], $domain = false ); + public function getLazyConnectionRef( $i, $groups = [], $domain = false ); + + /** + * Get a maintenance database connection handle reference for migrations and schema changes + * + * The handle's methods simply wrap those of a Database handle + * + * @see ILoadBalancer::getConnection() for parameter information + * + * @param int $db Server index or DB_MASTER/DB_REPLICA + * @param array|string|bool $groups Query group(s), or false for the generic reader + * @param string|bool $domain Domain ID, or false for the current domain + * @return MaintainableDBConnRef + */ + public function getMaintenanceConnectionRef( $db, $groups = [], $domain = false ); /** * Open a connection to the server given by the specified index @@ -216,9 +233,9 @@ interface ILoadBalancer { * * @note If disable() was called on this LoadBalancer, this method will throw a DBAccessError. * - * @param int $i Server index + * @param int $i Server index or DB_MASTER/DB_REPLICA * @param string|bool $domain Domain ID, or false for the current domain - * @return IDatabase|bool Returns false on errors + * @return Database|bool Returns false on errors * @throws DBAccessError */ public function openConnection( $i, $domain = false ); diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index 634993ac2e..8601785b19 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -657,6 +657,12 @@ class LoadBalancer implements ILoadBalancer { return new DBConnRef( $this, [ $db, $groups, $domain ] ); } + public function getMaintenanceConnectionRef( $db, $groups = [], $domain = false ) { + $domain = ( $domain !== false ) ? $domain : $this->localDomain; + + return new MaintainableDBConnRef( $this, $this->getConnection( $db, $groups, $domain ) ); + } + /** * @see ILoadBalancer::openConnection() * diff --git a/tests/phpunit/MediaWikiTestCase.php b/tests/phpunit/MediaWikiTestCase.php index db1df5c04f..76559afec6 100644 --- a/tests/phpunit/MediaWikiTestCase.php +++ b/tests/phpunit/MediaWikiTestCase.php @@ -1087,11 +1087,11 @@ abstract class MediaWikiTestCase extends PHPUnit_Framework_TestCase { * Clones all tables in the given database (whatever database that connection has * open), to versions with the test prefix. * - * @param Database $db Database to use + * @param IMaintainableDatabase $db Database to use * @param string $prefix Prefix to use for test tables * @return bool True if tables were cloned, false if only the prefix was changed */ - protected static function setupDatabaseWithTestPrefix( Database $db, $prefix ) { + protected static function setupDatabaseWithTestPrefix( IMaintainableDatabase $db, $prefix ) { $tablesCloned = self::listTables( $db ); $dbClone = new CloneDatabase( $db, $tablesCloned, $prefix ); $dbClone->useTemporaryTables( self::$useTemporaryTables ); @@ -1210,9 +1210,7 @@ abstract class MediaWikiTestCase extends PHPUnit_Framework_TestCase { list( $proto, $cluster ) = explode( '://', $url, 2 ); // Avoid getMaster() because setupDatabaseWithTestPrefix() // requires Database instead of plain DBConnRef/IDatabase - $lb = $externalStoreDB->getLoadBalancer( $cluster ); - $dbw = $lb->getConnection( DB_MASTER ); - $dbws[] = $dbw; + $dbws[] = $externalStoreDB->getMaster( $cluster ); } } @@ -1326,11 +1324,11 @@ abstract class MediaWikiTestCase extends PHPUnit_Framework_TestCase { /** * @since 1.18 * - * @param Database $db + * @param IMaintainableDatabase $db * * @return array */ - public static function listTables( Database $db ) { + public static function listTables( IMaintainableDatabase $db ) { $prefix = $db->tablePrefix(); $tables = $db->listTables( $prefix, __METHOD__ ); @@ -1378,6 +1376,8 @@ abstract class MediaWikiTestCase extends PHPUnit_Framework_TestCase { if ( isset( PHPUnitMaintClass::$additionalOptions[$offset] ) ) { return PHPUnitMaintClass::$additionalOptions[$offset]; } + + return null; } /** -- 2.20.1