Make insertSelect() do two separate queries in non-CLI mode

This avoids slave lag and makes query time account easier.
It also avoids table-level autoinc locking and slave drift
with statement-based replication in some setups.

Also refactored the use of $wgCommandLine mode in
DatabaseBase slightly, so that it can be injected.

Change-Id: I2dba6024ecf32c9ee24a3080cce3b02568c1458b

diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php
index fa6df8d..2b55f1b 100644 (file)
--- a/includes/DefaultSettings.php
+++ b/includes/DefaultSettings.php
@@ -1904,7 +1904,7 @@ $wgSharedSchema = false;
  *                  to several groups, the most specific group defined here is used.
  *
  *   - flags:       bit field
- *                  - DBO_DEFAULT -- turns on DBO_TRX only if !$wgCommandLineMode (recommended)
+ *                  - DBO_DEFAULT -- turns on DBO_TRX only if "cliMode" is off (recommended)
  *                  - DBO_DEBUG -- equivalent of $wgDebugDumpSql
  *                  - DBO_TRX -- wrap entire request in a transaction
  *                  - DBO_NOBUFFER -- turn off buffering (not useful in LocalSettings.php)
@@ -1915,6 +1915,9 @@ $wgSharedSchema = false;
  *
  *   - max lag:     (optional) Maximum replication lag before a slave will taken out of rotation
  *   - is static:   (optional) Set to true if the dataset is static and no replication is used.
+ *   - cliMode:     (optional) Connection handles will not assume that requests are short-lived
+ *                  nor that INSERT..SELECT can be rewritten into a buffered SELECT and INSERT.
+ *                  [Default: uses value of $wgCommandLineMode]
  *
  *   These and any other user-defined properties will be assigned to the mLBInfo member
  *   variable of the Database object.

diff --git a/includes/db/Database.php b/includes/db/Database.php
index 1adf73d..67afb83 100644 (file)
--- a/includes/db/Database.php
+++ b/includes/db/Database.php
@@ -59,6 +59,8 @@ abstract class DatabaseBase implements IDatabase {
        protected $mPassword;
        /** @var string */
        protected $mDBname;
+       /** @var bool */
+       protected $cliMode;
 
        /** @var BagOStuff APC cache */
        protected $srvCache;
@@ -568,7 +570,7 @@ abstract class DatabaseBase implements IDatabase {
         * @param array $params Parameters passed from DatabaseBase::factory()
         */
        function __construct( array $params ) {
-               global $wgDBprefix, $wgDBmwschema, $wgCommandLineMode;
+               global $wgDBprefix, $wgDBmwschema;
 
                $this->srvCache = ObjectCache::getLocalServerInstance( 'hash' );
 
@@ -581,9 +583,13 @@ abstract class DatabaseBase implements IDatabase {
                $schema = $params['schema'];
                $foreign = $params['foreign'];
 
+               $this->cliMode = isset( $params['cliMode'] )
+                       ? $params['cliMode']
+                       : ( PHP_SAPI === 'cli' );
+
                $this->mFlags = $flags;
                if ( $this->mFlags & DBO_DEFAULT ) {
-                       if ( $wgCommandLineMode ) {
+                       if ( $this->cliMode ) {
                                $this->mFlags &= ~DBO_TRX;
                        } else {
                                $this->mFlags |= DBO_TRX;
@@ -654,6 +660,8 @@ abstract class DatabaseBase implements IDatabase {
         * @return DatabaseBase|null DatabaseBase subclass or null
         */
        final public static function factory( $dbType, $p = [] ) {
+               global $wgCommandLineMode;
+
                $canonicalDBTypes = [
                        'mysql' => [ 'mysqli', 'mysql' ],
                        'postgres' => [],
@@ -711,6 +719,7 @@ abstract class DatabaseBase implements IDatabase {
                                $p['schema'] = isset( $defaultSchemas[$dbType] ) ? $defaultSchemas[$dbType] : null;
                        }
                        $p['foreign'] = isset( $p['foreign'] ) ? $p['foreign'] : false;
+                       $p['cliMode'] = $wgCommandLineMode;
 
                        return new $class( $p );
                } else {
@@ -1013,7 +1022,7 @@ abstract class DatabaseBase implements IDatabase {
         * queries, like inserting a row can take a long time due to row locking. This method
         * uses some simple heuristics to discount those cases.
         *
-        * @param string $sql
+        * @param string $sql A SQL write query
         * @param float $runtime Total runtime, including RTT
         */
        private function updateTrxWriteQueryTime( $sql, $runtime ) {
@@ -2411,7 +2420,46 @@ abstract class DatabaseBase implements IDatabase {
                return $this->query( $sql, $fname );
        }
 
-       public function insertSelect( $destTable, $srcTable, $varMap, $conds,
+       public function insertSelect(
+               $destTable, $srcTable, $varMap, $conds,
+               $fname = __METHOD__, $insertOptions = [], $selectOptions = []
+       ) {
+               if ( $this->cliMode ) {
+                       // For massive migrations with downtime, we don't want to select everything
+                       // into memory and OOM, so do all this native on the server side if possible.
+                       return $this->nativeInsertSelect(
+                               $destTable,
+                               $srcTable,
+                               $varMap,
+                               $conds,
+                               $fname,
+                               $insertOptions,
+                               $selectOptions
+                       );
+               }
+
+               // For web requests, do a locking SELECT and then INSERT. This puts the SELECT burden
+               // on only the master (without needing row-based-replication). It also makes it easy to
+               // know how big the INSERT is going to be.
+               $fields = [];
+               foreach ( $varMap as $dstColumn => $sourceColumnOrSql ) {
+                       $fields[] = $this->fieldNameWithAlias( $sourceColumnOrSql, $dstColumn );
+               }
+               $selectOptions[] = 'FOR UPDATE';
+               $res = $this->select( $srcTable, implode( ',', $fields ), $conds, $fname, $selectOptions );
+               if ( !$res ) {
+                       return false;
+               }
+
+               $rows = [];
+               foreach ( $res as $row ) {
+                       $rows[] = (array)$row;
+               }
+
+               return $this->insert( $destTable, $rows, $fname, $insertOptions );
+       }
+
+       public function nativeInsertSelect( $destTable, $srcTable, $varMap, $conds,
                $fname = __METHOD__,
                $insertOptions = [], $selectOptions = []
        ) {

diff --git a/includes/db/DatabaseMssql.php b/includes/db/DatabaseMssql.php
index 5e0365a..058c33e 100644 (file)
--- a/includes/db/DatabaseMssql.php
+++ b/includes/db/DatabaseMssql.php
@@ -730,12 +730,12 @@ class DatabaseMssql extends Database {
         * @return null|ResultWrapper
         * @throws Exception
         */
-       public function insertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__,
+       public function nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__,
                $insertOptions = [], $selectOptions = []
        ) {
                $this->mScrollableCursor = false;
                try {
-                       $ret = parent::insertSelect(
+                       $ret = parent::nativeInsertSelect(
                                $destTable,
                                $srcTable,
                                $varMap,

diff --git a/includes/db/DatabaseOracle.php b/includes/db/DatabaseOracle.php
index f9ba050..33a8280 100644 (file)
--- a/includes/db/DatabaseOracle.php
+++ b/includes/db/DatabaseOracle.php
@@ -732,7 +732,7 @@ class DatabaseOracle extends Database {
                return oci_free_statement( $stmt );
        }
 
-       function insertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__,
+       function nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__,
                $insertOptions = [], $selectOptions = []
        ) {
                $destTable = $this->tableName( $destTable );

diff --git a/includes/db/DatabasePostgres.php b/includes/db/DatabasePostgres.php
index 1ecdd26..c8e9ac7 100644 (file)
--- a/includes/db/DatabasePostgres.php
+++ b/includes/db/DatabasePostgres.php
@@ -904,7 +904,7 @@ __INDEXATTR__;
         * @param array $selectOptions
         * @return bool
         */
-       function insertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__,
+       function nativeInsertSelect( $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__,
                $insertOptions = [], $selectOptions = [] ) {
                $destTable = $this->tableName( $destTable );
 

diff --git a/tests/phpunit/includes/db/DatabaseSQLTest.php b/tests/phpunit/includes/db/DatabaseSQLTest.php
index 5d5521c..e7eeff9 100644 (file)
--- a/tests/phpunit/includes/db/DatabaseSQLTest.php
+++ b/tests/phpunit/includes/db/DatabaseSQLTest.php
@@ -5,15 +5,12 @@
  * This is a non DBMS depending test.
  */
 class DatabaseSQLTest extends MediaWikiTestCase {
-
-       /**
-        * @var DatabaseTestHelper
-        */
+       /** @var DatabaseTestHelper */
        private $database;
 
        protected function setUp() {
                parent::setUp();
-               $this->database = new DatabaseTestHelper( __CLASS__ );
+               $this->database = new DatabaseTestHelper( __CLASS__, [ 'cliMode' => true ] );
        }
 
        protected function assertLastSql( $sqlText ) {
@@ -23,6 +20,10 @@ class DatabaseSQLTest extends MediaWikiTestCase {
                );
        }
 
+       protected function assertLastSqlDb( $sqlText, $db ) {
+               $this->assertEquals( $db->getLastSqls(), $sqlText );
+       }
+
        /**
         * @dataProvider provideSelect
         * @covers DatabaseBase::select
@@ -354,7 +355,7 @@ class DatabaseSQLTest extends MediaWikiTestCase {
         * @dataProvider provideInsertSelect
         * @covers DatabaseBase::insertSelect
         */
-       public function testInsertSelect( $sql, $sqlText ) {
+       public function testInsertSelect( $sql, $sqlTextNative, $sqlSelect, $sqlInsert ) {
                $this->database->insertSelect(
                        $sql['destTable'],
                        $sql['srcTable'],
@@ -364,7 +365,22 @@ class DatabaseSQLTest extends MediaWikiTestCase {
                        isset( $sql['insertOptions'] ) ? $sql['insertOptions'] : [],
                        isset( $sql['selectOptions'] ) ? $sql['selectOptions'] : []
                );
-               $this->assertLastSql( $sqlText );
+               $this->assertLastSql( $sqlTextNative );
+
+               $dbWeb = new DatabaseTestHelper( __CLASS__, [ 'cliMode' => false ] );
+               $dbWeb->forceNextResult( [
+                       array_flip( array_keys( $sql['varMap'] ) )
+               ] );
+               $dbWeb->insertSelect(
+                       $sql['destTable'],
+                       $sql['srcTable'],
+                       $sql['varMap'],
+                       $sql['conds'],
+                       __METHOD__,
+                       isset( $sql['insertOptions'] ) ? $sql['insertOptions'] : [],
+                       isset( $sql['selectOptions'] ) ? $sql['selectOptions'] : []
+               );
+               $this->assertLastSqlDb( implode( '; ', [ $sqlSelect, $sqlInsert ] ), $dbWeb );
        }
 
        public static function provideInsertSelect() {
@@ -379,7 +395,10 @@ class DatabaseSQLTest extends MediaWikiTestCase {
                                "INSERT INTO insert_table " .
                                        "(field_insert,field) " .
                                        "SELECT field_select,field2 " .
-                                       "FROM select_table"
+                                       "FROM select_table",
+                               "SELECT field_select AS field_insert,field2 AS field " .
+                               "FROM select_table WHERE *   FOR UPDATE",
+                               "INSERT INTO insert_table (field_insert,field) VALUES ('0','1')"
                        ],
                        [
                                [
@@ -392,7 +411,10 @@ class DatabaseSQLTest extends MediaWikiTestCase {
                                        "(field_insert,field) " .
                                        "SELECT field_select,field2 " .
                                        "FROM select_table " .
-                                       "WHERE field = '2'"
+                                       "WHERE field = '2'",
+                               "SELECT field_select AS field_insert,field2 AS field FROM " .
+                               "select_table WHERE field = '2'   FOR UPDATE",
+                               "INSERT INTO insert_table (field_insert,field) VALUES ('0','1')"
                        ],
                        [
                                [
@@ -408,7 +430,10 @@ class DatabaseSQLTest extends MediaWikiTestCase {
                                        "SELECT field_select,field2 " .
                                        "FROM select_table " .
                                        "WHERE field = '2' " .
-                                       "ORDER BY field"
+                                       "ORDER BY field",
+                               "SELECT field_select AS field_insert,field2 AS field " .
+                               "FROM select_table WHERE field = '2' ORDER BY field  FOR UPDATE",
+                               "INSERT IGNORE INTO insert_table (field_insert,field) VALUES ('0','1')"
                        ],
                ];
        }

diff --git a/tests/phpunit/includes/db/DatabaseTestHelper.php b/tests/phpunit/includes/db/DatabaseTestHelper.php
index d6ca596..33ccb4d 100644 (file)
--- a/tests/phpunit/includes/db/DatabaseTestHelper.php
+++ b/tests/phpunit/includes/db/DatabaseTestHelper.php
@@ -19,17 +19,21 @@ class DatabaseTestHelper extends DatabaseBase {
         */
        protected $lastSqls = [];
 
+       /** @var array List of row arrays */
+       protected $nextResult = [];
+
        /**
         * Array of tables to be considered as existing by tableExist()
         * Use setExistingTables() to alter.
         */
        protected $tablesExists;
 
-       public function __construct( $testName ) {
+       public function __construct( $testName, array $opts = [] ) {
                $this->testName = $testName;
 
                $this->profiler = new ProfilerStub( [] );
                $this->trxProfiler = new TransactionProfiler();
+               $this->cliMode = isset( $opts['cliMode'] ) ? $opts['cliMode'] : true;
        }
 
        /**
@@ -47,6 +51,13 @@ class DatabaseTestHelper extends DatabaseBase {
                $this->tablesExists = (array)$tablesExists;
        }
 
+       /**
+        * @param mixed $res Use an array of row arrays to set row result
+        */
+       public function forceNextResult( $res ) {
+               $this->nextResult = $res;
+       }
+
        protected function addSql( $sql ) {
                // clean up spaces before and after some words and the whole string
                $this->lastSqls[] = trim( preg_replace(
@@ -168,6 +179,9 @@ class DatabaseTestHelper extends DatabaseBase {
        }
 
        protected function doQuery( $sql ) {
-               return [];
+               $res = $this->nextResult;
+               $this->nextResult = [];
+
+               return new FakeResultWrapper( $res );
        }
 }