From 52d2ebb30a0fc801cd1dcc952bec7cbcf73ceb53 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sat, 27 Aug 2016 10:10:46 -0700 Subject: [PATCH] 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 --- includes/DefaultSettings.php | 5 +- includes/db/Database.php | 56 +++++++++++++++++-- includes/db/DatabaseMssql.php | 4 +- includes/db/DatabaseOracle.php | 2 +- includes/db/DatabasePostgres.php | 2 +- tests/phpunit/includes/db/DatabaseSQLTest.php | 45 +++++++++++---- .../includes/db/DatabaseTestHelper.php | 18 +++++- 7 files changed, 111 insertions(+), 21 deletions(-) diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index fa6df8d6c0..2b55f1b4bd 100644 --- 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 1adf73df95..67afb83b8a 100644 --- 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 5e0365a2d8..058c33e1c8 100644 --- 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 f9ba0507d4..33a8280410 100644 --- 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 1ecdd26088..c8e9ac7ff6 100644 --- 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 5d5521ccfe..e7eeff9429 100644 --- 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 d6ca5964b8..33ccb4d201 100644 --- 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 ); } } -- 2.20.1