From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Fri, 26 Jan 2018 06:02:22 +0000 (-0800)
Subject: Make DatabaseMysqlBase::insertSelect() safer to use
X-Git-Tag: 1.31.0-rc.0~774^2
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=d40916cb85ebfe46cf87d408472cf01023d13812

Make DatabaseMysqlBase::insertSelect() safer to use

Certain server configurations, including the current MariaDB defaults,
make INSERT SELECT unsafe for replication in MySQL/MariaDB. When the
server configuration is not known to be safe, force the use of the
non-native implementation. Note this only has effect in CLI mode, as
non-CLI mode already forces the non-native implementation since
I2dba6024.

Also, native INSERT SELECT won't be safe with any statement-based
replication method if the order of rows in the SELECT is not
deterministic. Add a warning to the method's documentation pointing this
out.

Change-Id: I9173f6559809bd01830bd0a9f443c7269cc58ce2
---

diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php
index 305a056900..82a78cab46 100644
--- a/includes/libs/rdbms/database/DatabaseMysqlBase.php
+++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php
@@ -63,6 +63,8 @@ abstract class DatabaseMysqlBase extends Database {
 
 	/** @var string|null */
 	private $serverVersion = null;
+	/** @var bool|null */
+	private $insertSelectIsSafe = null;
 
 	/**
 	 * Additional $params include:
@@ -75,6 +77,7 @@ abstract class DatabaseMysqlBase extends Database {
 	 *       ID of this server's master will be used. Set the "conds" field to
 	 *       override the query conditions, e.g. ['shard' => 's1'].
 	 *   - useGTIDs : use GTID methods like MASTER_GTID_WAIT() when possible.
+	 *   - insertSelectIsSafe : force that native INSERT SELECT is or is not safe [default: null]
 	 *   - sslKeyPath : path to key file [default: null]
 	 *   - sslCertPath : path to certificate file [default: null]
 	 *   - sslCAFile: path to a single certificate authority PEM file [default: null]
@@ -98,6 +101,8 @@ abstract class DatabaseMysqlBase extends Database {
 		}
 		$this->sqlMode = isset( $params['sqlMode'] ) ? $params['sqlMode'] : '';
 		$this->utf8Mode = !empty( $params['utf8Mode'] );
+		$this->insertSelectIsSafe = isset( $params['insertSelectIsSafe'] )
+			? (bool)$params['insertSelectIsSafe'] : null;
 
 		parent::__construct( $params );
 	}
@@ -501,6 +506,56 @@ abstract class DatabaseMysqlBase extends Database {
 		return $this->nativeReplace( $table, $rows, $fname );
 	}
 
+	protected function nativeInsertSelect(
+		$destTable, $srcTable, $varMap, $conds,
+		$fname = __METHOD__, $insertOptions = [], $selectOptions = [], $selectJoinConds = []
+	) {
+		if ( $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(
+				false,
+				[
+					'innodb_autoinc_lock_mode' => '@@innodb_autoinc_lock_mode',
+					'binlog_format' => '@@binlog_format',
+				],
+				[],
+				__METHOD__
+			);
+			$this->insertSelectIsSafe = $row->binlog_format === 'ROW' ||
+				(int)$row->innodb_autoinc_lock_mode === 0;
+		}
+
+		if ( !$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
+			);
+		}
+	}
+
 	/**
 	 * Estimate rows in dataset
 	 * Returns estimated count, based on EXPLAIN output
diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php
index 85b3481fe3..0964dd55fe 100644
--- a/includes/libs/rdbms/database/IDatabase.php
+++ b/includes/libs/rdbms/database/IDatabase.php
@@ -1255,6 +1255,11 @@ interface IDatabase {
 	 * INSERT SELECT wrapper. Takes data from a SELECT query and inserts it
 	 * into another table.
 	 *
+	 * @warning If the insert will use an auto-increment or sequence to
+	 *  determine the value of a column, this may break replication on
+	 *  databases using statement-based replication if the SELECT is not
+	 *  deterministically ordered.
+	 *
 	 * @param string $destTable The table name to insert into
 	 * @param string|array $srcTable May be either a table name, or an array of table names
 	 *    to include in a join.