From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 28 Feb 2018 23:33:03 +0000 (-0800)
Subject: rdbms: make Database::insertSelect() stricter about replication safety
X-Git-Tag: 1.31.0-rc.0~470^2
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=671368a59e3820109a2bd63fc2cbc2331495f952

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 2992e7628e..d5fc357f64 100644
--- 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 );
 	}
 
-	public function insertSelect(
+	final public function insertSelect(
 		$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(
@@ -2459,7 +2464,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 				$varMap,
 				$conds,
 				$fname,
-				$insertOptions,
+				array_diff( $insertOptions, $hints ),
 				$selectOptions,
 				$selectJoinConds
 			);
@@ -2471,12 +2476,22 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware
 			$varMap,
 			$conds,
 			$fname,
-			$insertOptions,
+			array_diff( $insertOptions, $hints ),
 			$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()
 	 *
@@ -2496,8 +2511,6 @@ 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.
@@ -2574,7 +2587,6 @@ 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 454e0c2ade..1e845e8d86 100644
--- 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;
+	/** @var stdClass|null */
+	private $replicationInfoRow = null;
 
 	/**
 	 * Additional $params include:
@@ -508,20 +510,32 @@ abstract class DatabaseMysqlBase extends Database {
 		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',
@@ -530,33 +544,9 @@ abstract class DatabaseMysqlBase extends Database {
 				[],
 				__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;
 	}
 
 	/**