rdbms: make Database::insertSelect() stricter about replication safety

Avoid the native INSERT SELECT method if a LIMIT clause is present.

Change-Id: Ibf9b8a4a42092fbc98d7ebd45167203a6a8801ee

diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php
index 2992e76..d5fc357 100644 (file)
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -2446,11 +2446,16 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                return $this->query( $sql, $fname );
        }
 
                return $this->query( $sql, $fname );
        }
 

-       public function insertSelect(
+       final public function insertSelect(

                $destTable, $srcTable, $varMap, $conds,
                $fname = __METHOD__, $insertOptions = [], $selectOptions = [], $selectJoinConds = []
        ) {
                $destTable, $srcTable, $varMap, $conds,
                $fname = __METHOD__, $insertOptions = [], $selectOptions = [], $selectJoinConds = []
        ) {

-               if ( $this->cliMode ) {
+               static $hints = [ 'NO_AUTO_COLUMNS' ];
+
+               $insertOptions = (array)$insertOptions;
+               $selectOptions = (array)$selectOptions;
+
+               if ( $this->cliMode && $this->isInsertSelectSafe( $insertOptions, $selectOptions ) ) {

                        // 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(
                        // 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(


@@ -2459,7 +2464,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                                $varMap,
                                $conds,
                                $fname,
                                $varMap,
                                $conds,
                                $fname,

-                               $insertOptions,
+                               array_diff( $insertOptions, $hints ),

                                $selectOptions,
                                $selectJoinConds
                        );
                                $selectOptions,
                                $selectJoinConds
                        );


@@ -2471,12 +2476,22 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                        $varMap,
                        $conds,
                        $fname,
                        $varMap,
                        $conds,
                        $fname,

-                       $insertOptions,
+                       array_diff( $insertOptions, $hints ),

                        $selectOptions,
                        $selectJoinConds
                );
        }
 
                        $selectOptions,
                        $selectJoinConds
                );
        }
 

+       /**
+        * @param array $insertOptions INSERT options
+        * @param array $selectOptions SELECT options
+        * @return bool Whether an INSERT SELECT with these options will be replication safe
+        * @since 1.31
+        */
+       protected function isInsertSelectSafe( array $insertOptions, array $selectOptions ) {
+               return true;
+       }
+

        /**
         * Implementation of insertSelect() based on select() and insert()
         *
        /**
         * Implementation of insertSelect() based on select() and insert()
         *


@@ -2496,8 +2511,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                $fname = __METHOD__,
                $insertOptions = [], $selectOptions = [], $selectJoinConds = []
        ) {
                $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.
                // 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.


@@ -2574,7 +2587,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
                if ( !is_array( $insertOptions ) ) {
                        $insertOptions = [ $insertOptions ];
                }
                if ( !is_array( $insertOptions ) ) {
                        $insertOptions = [ $insertOptions ];
                }

-               $insertOptions = array_diff( (array)$insertOptions, [ 'NO_AUTO_COLUMNS' ] );

 
                $insertOptions = $this->makeInsertOptions( $insertOptions );
 
 
                $insertOptions = $this->makeInsertOptions( $insertOptions );
 

diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php
index 454e0c2..1e845e8 100644 (file)
--- a/includes/libs/rdbms/database/DatabaseMysqlBase.php
+++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php
@@ -67,6 +67,8 @@ abstract class DatabaseMysqlBase extends Database {
        private $serverVersion = null;
        /** @var bool|null */
        private $insertSelectIsSafe = null;
        private $serverVersion = null;
        /** @var bool|null */
        private $insertSelectIsSafe = null;

+       /** @var stdClass|null */
+       private $replicationInfoRow = null;

 
        /**
         * Additional $params include:
 
        /**
         * Additional $params include:


@@ -508,20 +510,32 @@ abstract class DatabaseMysqlBase extends Database {
                return $this->nativeReplace( $table, $rows, $fname );
        }
 
                return $this->nativeReplace( $table, $rows, $fname );
        }
 

-       protected function nativeInsertSelect(
-               $destTable, $srcTable, $varMap, $conds,
-               $fname = __METHOD__, $insertOptions = [], $selectOptions = [], $selectJoinConds = []
-       ) {
-               $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.
-                       // While we could try to determine if the insert is safe anyway by
-                       // checking if the target table has an auto-increment column that
-                       // isn't set in $varMap, that seems unlikely to be worth the extra
-                       // complexity.
-                       $row = $this->selectRow(
+       protected function isInsertSelectSafe( array $insertOptions, array $selectOptions ) {
+               $row = $this->getReplicationSafetyInfo();
+               // For row-based-replication, the resulting changes will be relayed, not the query
+               if ( $row->binlog_format === 'ROW' ) {
+                       return true;
+               }
+               // LIMIT requires ORDER BY on a unique key or it is non-deterministic
+               if ( isset( $selectOptions['LIMIT'] ) ) {
+                       return false;
+               }
+               // 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.
+               // While we could try to determine if the insert is safe anyway by
+               // checking if the target table has an auto-increment column that
+               // isn't set in $varMap, that seems unlikely to be worth the extra
+               // complexity.
+               return ( (int)$row->innodb_autoinc_lock_mode === 0 );
+       }
+
+       /**
+        * @return stdClass Process cached row
+        */
+       private function getReplicationSafetyInfo() {
+               if ( $this->replicationInfoRow === null ) {
+                       $this->replicationInfoRow = $this->selectRow(

                                false,
                                [
                                        'innodb_autoinc_lock_mode' => '@@innodb_autoinc_lock_mode',
                                false,
                                [
                                        'innodb_autoinc_lock_mode' => '@@innodb_autoinc_lock_mode',


@@ -530,33 +544,9 @@ abstract class DatabaseMysqlBase extends Database {
                                [],
                                __METHOD__
                        );
                                [],
                                __METHOD__
                        );

-                       $this->insertSelectIsSafe = $row->binlog_format === 'ROW' ||
-                               (int)$row->innodb_autoinc_lock_mode === 0;
-               }
-
-               if ( !$isSafe && !$this->insertSelectIsSafe ) {
-                       return $this->nonNativeInsertSelect(
-                               $destTable,
-                               $srcTable,
-                               $varMap,
-                               $conds,
-                               $fname,
-                               $insertOptions,
-                               $selectOptions,
-                               $selectJoinConds
-                       );
-               } else {
-                       return parent::nativeInsertSelect(
-                               $destTable,
-                               $srcTable,
-                               $varMap,
-                               $conds,
-                               $fname,
-                               $insertOptions,
-                               $selectOptions,
-                               $selectJoinConds
-                       );

                }
                }

+
+               return $this->replicationInfoRow;

        }
 
        /**
        }
 
        /**