Add LoadBalancer::getMaintenanceConnectionRef() method
authorAaron Schulz <aschulz@wikimedia.org>
Mon, 28 Nov 2016 18:26:14 +0000 (10:26 -0800)
committerAaron Schulz <aschulz@wikimedia.org>
Sat, 10 Dec 2016 23:35:09 +0000 (15:35 -0800)
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
includes/db/CloneDatabase.php
includes/externalstore/ExternalStoreDB.php
includes/libs/rdbms/database/DBConnRef.php
includes/libs/rdbms/database/Database.php
includes/libs/rdbms/database/IMaintainableDatabase.php
includes/libs/rdbms/database/MaintainableDBConnRef.php [new file with mode: 0644]
includes/libs/rdbms/loadbalancer/ILoadBalancer.php
includes/libs/rdbms/loadbalancer/LoadBalancer.php
tests/phpunit/MediaWikiTestCase.php

index e079686..66a9e9b 100644 (file)
@@ -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',
index f1ccd2a..2b394b6 100644 (file)
@@ -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 );
                }
        }
 
index 7e93299..52c1a4c 100644 (file)
@@ -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__ );
index 20198bf..b268b9f 100644 (file)
 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 );
                }
        }
index 3d35d76..b9beac8 100644 (file)
@@ -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' );
        }
index 8395359..43cec28 100644 (file)
@@ -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 (file)
index 0000000..fa3ddf9
--- /dev/null
@@ -0,0 +1,68 @@
+<?php
+/**
+ * Helper class to handle automatically marking connections as reusable (via RAII pattern)
+ * as well handling deferring the actual network connection until the handle is used
+ *
+ * @note: proxy methods are defined explicity to avoid interface errors
+ * @ingroup Database
+ * @since 1.29
+ */
+class MaintainableDBConnRef extends DBConnRef implements IMaintainableDatabase {
+       public function tableName( $name, $format = 'quoted' ) {
+               return $this->__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() );
+       }
+}
index 8854479..fc306b4 100644 (file)
@@ -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 );
index 634993a..8601785 100644 (file)
@@ -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()
         *
index db1df5c..76559af 100644 (file)
@@ -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;
        }
 
        /**