From: Brad Jorsch Date: Thu, 8 Feb 2018 19:18:24 +0000 (-0500) Subject: rdbms: allow callers to hint that native insertSelect() is safe X-Git-Tag: 1.31.0-rc.0~482^2 X-Git-Url: http://git.cyclocoop.org/?a=commitdiff_plain;h=99b65649c0f9b4ab1156cfc2ee50959d4b8a12c6;p=lhc%2Fweb%2Fwiklou.git rdbms: allow callers to hint that native insertSelect() is safe An INSERT SELECT in MySQL/MariaDB is unsafe for replication if a column is getting values from auto-increment, statement-based replication is in use, and the default innodb_autoinc_lock_mode is set. I9173f655 added checks to force non-native insertSelect for the statement-based replication and innodb_autoinc_lock_mode != 2 case, but determining whether a column is getting values from auto-increment is too hard to do automatically there. Instead, let's add a flag to let the caller hint that the query isn't getting any auto-increment values. And use it in MysqlUpdater when appropriate. Bug: T160993 Change-Id: If70450a64aa3bcbf763c62838bb21306d124ae3d --- diff --git a/includes/installer/MysqlUpdater.php b/includes/installer/MysqlUpdater.php index 350962fd1e..bce469053b 100644 --- a/includes/installer/MysqlUpdater.php +++ b/includes/installer/MysqlUpdater.php @@ -474,6 +474,17 @@ class MysqlUpdater extends DatabaseUpdater { return; } + $insertOpts = [ 'IGNORE' ]; + $selectOpts = []; + + // If wl_id exists, make insertSelect() more replication-safe by + // ordering on that column. If not, hint that it should be safe anyway. + if ( $this->db->fieldExists( 'watchlist', 'wl_id', __METHOD__ ) ) { + $selectOpts['ORDER BY'] = 'wl_id'; + } else { + $insertOpts[] = 'NO_AUTO_COLUMNS'; + } + $this->output( "Adding missing watchlist talk page rows... " ); $this->db->insertSelect( 'watchlist', 'watchlist', [ @@ -481,7 +492,7 @@ class MysqlUpdater extends DatabaseUpdater { 'wl_namespace' => 'wl_namespace | 1', 'wl_title' => 'wl_title', 'wl_notificationtimestamp' => 'wl_notificationtimestamp' - ], [ 'NOT (wl_namespace & 1)' ], __METHOD__, 'IGNORE' ); + ], [ 'NOT (wl_namespace & 1)' ], __METHOD__, $insertOpts, $selectOpts ); $this->output( "done.\n" ); $this->output( "Adding missing watchlist subject page rows... " ); @@ -491,7 +502,7 @@ class MysqlUpdater extends DatabaseUpdater { 'wl_namespace' => 'wl_namespace & ~1', 'wl_title' => 'wl_title', 'wl_notificationtimestamp' => 'wl_notificationtimestamp' - ], [ 'wl_namespace & 1' ], __METHOD__, 'IGNORE' ); + ], [ 'wl_namespace & 1' ], __METHOD__, $insertOpts, $selectOpts ); $this->output( "done.\n" ); } @@ -903,7 +914,8 @@ class MysqlUpdater extends DatabaseUpdater { 'tl_title' => 'pl_title' ], [ 'pl_namespace' => 10 - ], __METHOD__ + ], __METHOD__, + [ 'NO_AUTO_COLUMNS' ] // There's no "tl_id" auto-increment field ); } $this->output( "Done. Please run maintenance/refreshLinks.php for a more " . diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index fd7bfe96bd..df28f93364 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -2497,6 +2497,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $fname = __METHOD__, $insertOptions = [], $selectOptions = [], $selectJoinConds = [] ) { + $insertOptions = array_diff( (array)$insertOptions, [ 'NO_AUTO_COLUMNS' ] ); + // 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. @@ -2575,6 +2577,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware if ( !is_array( $insertOptions ) ) { $insertOptions = [ $insertOptions ]; } + $insertOptions = array_diff( (array)$insertOptions, [ 'NO_AUTO_COLUMNS' ] ); $insertOptions = $this->makeInsertOptions( $insertOptions ); diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php index 57b75445ef..454e0c2ade 100644 --- a/includes/libs/rdbms/database/DatabaseMysqlBase.php +++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php @@ -512,7 +512,8 @@ abstract class DatabaseMysqlBase extends Database { $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__, $insertOptions = [], $selectOptions = [], $selectJoinConds = [] ) { - if ( $this->insertSelectIsSafe === null ) { + $isSafe = in_array( 'NO_AUTO_COLUMNS', $insertOptions, true ); + if ( !$isSafe && $this->insertSelectIsSafe === null ) { // In MySQL, an INSERT SELECT is only replication safe with row-based // replication or if innodb_autoinc_lock_mode is 0. When those // conditions aren't met, use non-native mode. @@ -533,7 +534,7 @@ abstract class DatabaseMysqlBase extends Database { (int)$row->innodb_autoinc_lock_mode === 0; } - if ( !$this->insertSelectIsSafe ) { + if ( !$isSafe && !$this->insertSelectIsSafe ) { return $this->nonNativeInsertSelect( $destTable, $srcTable, diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php index 2922bce327..9ad78a7868 100644 --- a/includes/libs/rdbms/database/IDatabase.php +++ b/includes/libs/rdbms/database/IDatabase.php @@ -1276,7 +1276,9 @@ interface IDatabase { * @param string $fname The function name of the caller, from __METHOD__ * * @param array $insertOptions Options for the INSERT part of the query, see - * IDatabase::insert() for details. + * IDatabase::insert() for details. Also, one additional option is + * available: pass 'NO_AUTO_COLUMNS' to hint that the query does not use + * an auto-increment or sequence to determine any column values. * @param array $selectOptions Options for the SELECT part of the query, see * IDatabase::select() for details. * @param array $selectJoinConds Join conditions for the SELECT part of the query, see diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php index 888654411a..3d1fe1a1a6 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseSQLTest.php @@ -518,6 +518,7 @@ class DatabaseSQLTest extends PHPUnit\Framework\TestCase { 'srcTable' => [ 'select_table1', 'select_table2' ], 'varMap' => [ 'field_insert' => 'field_select', 'field' => 'field2' ], 'conds' => [ 'field' => 2 ], + 'insertOptions' => [ 'NO_AUTO_COLUMNS' ], 'selectOptions' => [ 'ORDER BY' => 'field', 'FORCE INDEX' => [ 'select_table1' => 'index1' ] ], 'selectJoinConds' => [ 'select_table2' => [ 'LEFT JOIN', [ 'select_table1.foo = select_table2.bar' ] ],