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 350962f..bce4690 100644 (file)
--- 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 fd7bfe9..df28f93 100644 (file)
--- 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 57b7544..454e0c2 100644 (file)
--- 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 2922bce..9ad78a7 100644 (file)
--- 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 8886544..3d1fe1a 100644 (file)
--- 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' ] ],