MDEV-36330: SERIALIZABLE read inconsistency

At TRANSACTION ISOLATION LEVEL SERIALIZABLE, InnoDB would fail to flag
a write/read conflict, which would be a violation already at the more
relaxed REPEATABLE READ level when innodb_snapshot_isolation=ON.

Fix: Create a read view and start the transaction at the same time.
Thus, lock checks will be able to consult the correct read view
to flag ER_CHECKREAD if we are about to lock a record that was committed
after the start of our transaction.

innobase_start_trx_and_assign_read_view(): At any other isolation level
than READ UNCOMMITTED, do create a read view. This is needed for the
correct operation of START TRANSACTION WITH CONSISTENT SNAPSHOT.

ha_innobase::store_lock(): At SERIALIZABLE isolation level, if the
transaction was not started yet, start it and open a read view.
An alternative way to achieve this would be to make trans_begin()
treat START TRANSACTION (or BEGIN) in the same way as
START TRANSACTION WITH CONSISTENT SNAPSHOT when the isolation level
is SERIALIZABLE.

innodb_isolation_level(const THD*): A simpler version of
innobase_map_isolation_level(). Compared to earlier, we will return
READ UNCOMMITTED also if the :newraw option is set for the
InnoDB system tablespace.

Reviewed by: Vladislav Lesin

mysql-test/suite/innodb/r/lock_isolation.result

@@ -166,7 +166,6 @@ SELECT * FROM t FORCE INDEX (b) FOR UPDATE;
 a	b
 1	NULL
 COMMIT;
-disconnect con_weird;
 connection consistent;
 SELECT * FROM t FORCE INDEX (b) FOR UPDATE;
 a	b
@@ -230,9 +229,58 @@ UPDATE t SET b=4 WHERE a=1;
 connection consistent;
 SELECT * FROM t WHERE a=1 FOR UPDATE;
 ERROR HY000: Record has changed since last read in table 't'
-disconnect consistent;
 disconnect disable_purging;
 connection default;
 SET DEBUG_SYNC="RESET";
 DROP TABLE t;
+CREATE TABLE t1(a INT) ENGINE=InnoDB STATS_PERSISTENT=0;
+CREATE TABLE t2(a INT) ENGINE=InnoDB STATS_PERSISTENT=0;
+BEGIN;
+INSERT INTO t1 SET a=1;
+connection con_weird;
+SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+BEGIN;
+INSERT INTO t2 SET a=1;
+connection consistent;
+SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+BEGIN;
+INSERT INTO t2 SET a=2;
+connection default;
+COMMIT;
+connection con_weird;
+SELECT * FROM t1;
+a
+1
+COMMIT;
+connection consistent;
+SELECT * FROM t1;
+ERROR HY000: Record has changed since last read in table 't1'
+COMMIT;
+connection default;
+BEGIN;
+INSERT INTO t1 SET a=2;
+connection con_weird;
+SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+START TRANSACTION WITH CONSISTENT SNAPSHOT;
+INSERT INTO t2 SET a=3;
+connection consistent;
+SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+START TRANSACTION WITH CONSISTENT SNAPSHOT;
+INSERT INTO t2 SET a=2;
+connection default;
+COMMIT;
+connection con_weird;
+SELECT * FROM t1;
+a
+1
+2
+COMMIT;
+disconnect con_weird;
+connection consistent;
+SELECT * FROM t1;
+ERROR HY000: Record has changed since last read in table 't1'
+COMMIT;
+disconnect consistent;
+connection default;
+DROP TABLE t1,t2;
 # End of 10.6 tests

mysql-test/suite/innodb/t/lock_isolation.test

@@ -174,7 +174,6 @@ ROLLBACK;
 --reap
 SELECT * FROM t FORCE INDEX (b) FOR UPDATE;
 COMMIT;
---disconnect con_weird
 
 --connection consistent
 SELECT * FROM t FORCE INDEX (b) FOR UPDATE;
@@ -246,12 +245,55 @@ UPDATE t SET b=4 WHERE a=1;
 --connection consistent
 --error ER_CHECKREAD
 SELECT * FROM t WHERE a=1 FOR UPDATE;
---disconnect consistent
 --disconnect disable_purging
 
 --connection default
 SET DEBUG_SYNC="RESET";
 DROP TABLE t;
 
+CREATE TABLE t1(a INT) ENGINE=InnoDB STATS_PERSISTENT=0;
+CREATE TABLE t2(a INT) ENGINE=InnoDB STATS_PERSISTENT=0;
+BEGIN; INSERT INTO t1 SET a=1;
+--connection con_weird
+SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+BEGIN; INSERT INTO t2 SET a=1;
+--connection consistent
+SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+BEGIN; INSERT INTO t2 SET a=2;
+--connection default
+COMMIT;
+--connection con_weird
+SELECT * FROM t1;
+COMMIT;
+--connection consistent
+--disable_ps2_protocol
+--error ER_CHECKREAD
+SELECT * FROM t1;
+--enable_ps2_protocol
+COMMIT;
+--connection default
+BEGIN; INSERT INTO t1 SET a=2;
+--connection con_weird
+SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+START TRANSACTION WITH CONSISTENT SNAPSHOT; INSERT INTO t2 SET a=3;
+--connection consistent
+SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+START TRANSACTION WITH CONSISTENT SNAPSHOT; INSERT INTO t2 SET a=2;
+--connection default
+COMMIT;
+--connection con_weird
+SELECT * FROM t1;
+COMMIT;
+--disconnect con_weird
+--connection consistent
+--disable_ps2_protocol
+--error ER_CHECKREAD
+SELECT * FROM t1;
+--enable_ps2_protocol
+COMMIT;
+--disconnect consistent
+--connection default
+DROP TABLE t1,t2;
+
 --source include/wait_until_count_sessions.inc
 --echo # End of 10.6 tests

storage/innobase/handler/ha_innodb.cc

@@ -116,8 +116,6 @@ simple_thread_local ha_handler_stats *mariadb_stats;
 #include <limits>
 #include <myisamchk.h>                          // TT_FOR_UPGRADE
 
-#define thd_get_trx_isolation(X) ((enum_tx_isolation)thd_tx_isolation(X))
-
 extern "C" void thd_mark_transaction_to_rollback(MYSQL_THD thd, bool all);
 unsigned long long thd_get_query_id(const MYSQL_THD thd);
 void thd_clear_error(MYSQL_THD thd);
@@ -821,14 +819,16 @@ innodb_tmpdir_validate(
 	return(0);
 }
 
-/******************************************************************//**
-Maps a MySQL trx isolation level code to the InnoDB isolation level code
-@return	InnoDB isolation level */
-static inline
-uint
-innobase_map_isolation_level(
-/*=========================*/
-	enum_tx_isolation	iso);	/*!< in: MySQL isolation level code */
+/** @return the current transaction isolation level */
+static inline uint innodb_isolation_level(const THD *thd) noexcept
+{
+  static_assert(ISO_REPEATABLE_READ == TRX_ISO_REPEATABLE_READ, "");
+  static_assert(ISO_SERIALIZABLE == TRX_ISO_SERIALIZABLE, "");
+  static_assert(ISO_READ_COMMITTED == TRX_ISO_READ_COMMITTED, "");
+  static_assert(ISO_READ_UNCOMMITTED == TRX_ISO_READ_UNCOMMITTED, "");
+  return high_level_read_only
+    ? ISO_READ_UNCOMMITTED : (thd_tx_isolation(thd) & 3);
+}
 
 /** Gets field offset for a field in a table.
 @param[in]	table	MySQL table object
@@ -4470,21 +4470,18 @@ innobase_start_trx_and_assign_read_view(
 
 	trx_start_if_not_started_xa(trx, false);
 
-	/* Assign a read view if the transaction does not have it yet.
-	Do this only if transaction is using REPEATABLE READ isolation
-	level. */
-	trx->isolation_level = innobase_map_isolation_level(
-		thd_get_trx_isolation(thd)) & 3;
+	/* Assign a read view if the transaction does not have one yet.
+	Skip this for the READ UNCOMMITTED isolation level. */
+	trx->isolation_level = innodb_isolation_level(thd) & 3;
 
-	if (trx->isolation_level == TRX_ISO_REPEATABLE_READ) {
+	if (trx->isolation_level != TRX_ISO_READ_UNCOMMITTED) {
 		trx->read_view.open(trx);
 	} else {
 		push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
 				    HA_ERR_UNSUPPORTED,
 				    "InnoDB: WITH CONSISTENT SNAPSHOT"
-				    " was ignored because this phrase"
-				    " can only be used with"
-				    " REPEATABLE READ isolation level.");
+				    " is ignored at READ UNCOMMITTED"
+				    " isolation level.");
 	}
 
 	/* Set the MySQL flag to mark that there is an active transaction */
@@ -16031,31 +16028,6 @@ ha_innobase::start_stmt(
 	DBUG_RETURN(0);
 }
 
-/******************************************************************//**
-Maps a MySQL trx isolation level code to the InnoDB isolation level code
-@return InnoDB isolation level */
-static inline
-uint
-innobase_map_isolation_level(
-/*=========================*/
-	enum_tx_isolation	iso)	/*!< in: MySQL isolation level code */
-{
-	if (UNIV_UNLIKELY(srv_force_recovery >= SRV_FORCE_NO_UNDO_LOG_SCAN)
-	    || UNIV_UNLIKELY(srv_read_only_mode)) {
-		return TRX_ISO_READ_UNCOMMITTED;
-	}
-	switch (iso) {
-	case ISO_REPEATABLE_READ:	return(TRX_ISO_REPEATABLE_READ);
-	case ISO_READ_COMMITTED:	return(TRX_ISO_READ_COMMITTED);
-	case ISO_SERIALIZABLE:		return(TRX_ISO_SERIALIZABLE);
-	case ISO_READ_UNCOMMITTED:	return(TRX_ISO_READ_UNCOMMITTED);
-	}
-
-	ut_error;
-
-	return(0);
-}
-
 /******************************************************************//**
 As MySQL will execute an external lock for every new table it uses when it
 starts to process an SQL statement (an exception is when MySQL calls
@@ -16520,19 +16492,29 @@ ha_innobase::store_lock(
 	Be careful to ignore TL_IGNORE if we are going to do something with
 	only 'real' locks! */
 
-	/* If no MySQL table is in use, we need to set the isolation level
+	/* If no table handle is open, we need to set the isolation level
 	of the transaction. */
 
 	if (lock_type != TL_IGNORE
 	    && trx->n_mysql_tables_in_use == 0) {
-		trx->isolation_level = innobase_map_isolation_level(
-			(enum_tx_isolation) thd_tx_isolation(thd)) & 3;
-
-		if (trx->isolation_level <= TRX_ISO_READ_COMMITTED) {
-
+		switch ((trx->isolation_level
+			 = innodb_isolation_level(thd) & 3)) {
+		case ISO_REPEATABLE_READ:
+			break;
+		case ISO_READ_COMMITTED:
+		case ISO_READ_UNCOMMITTED:
 			/* At low transaction isolation levels we let
 			each consistent read set its own snapshot */
 			trx->read_view.close();
+			break;
+		case ISO_SERIALIZABLE:
+			auto trx_state = trx->state;
+			if (trx_state == TRX_STATE_NOT_STARTED) {
+				trx_start_if_not_started(trx, false);
+				trx->read_view.open(trx);
+			} else {
+				ut_ad(trx_state == TRX_STATE_ACTIVE);
+			}
 		}
 	}
 

storage/innobase/mysql-test/storage_engine/trx/cons_snapshot_serializable.rdiff

@@ -1,14 +1,6 @@
 --- suite/storage_engine/trx/cons_snapshot_serializable.result
 +++ suite/storage_engine/trx/cons_snapshot_serializable.reject
-@@ -5,12 +5,15 @@
- CREATE TABLE t1 (a <INT_COLUMN>) ENGINE=<STORAGE_ENGINE> <CUSTOM_TABLE_OPTIONS>;
- SET SESSION TRANSACTION ISOLATION LEVEL SERIALIZABLE;
- START TRANSACTION WITH CONSISTENT SNAPSHOT;
-+Warnings:
-+Warning	138	InnoDB: WITH CONSISTENT SNAPSHOT was ignored because this phrase can only be used with REPEATABLE READ isolation level.
- connection con2;
- INSERT INTO t1 (a) VALUES (1);
- connection con1;
+@@ -11,6 +11,7 @@
  # If consistent read works on this isolation level (SERIALIZABLE), the following SELECT should not return the value we inserted (1)
  SELECT a FROM t1;
  a

storage/innobase/mysql-test/storage_engine/trx/level_read_committed.rdiff

@@ -1,11 +0,0 @@
---- suite/storage_engine/trx/level_read_committed.result
-+++ suite/storage_engine/trx/level_read_committed.reject
-@@ -77,6 +77,8 @@
- CREATE TABLE t1 (a <INT_COLUMN>) ENGINE=<STORAGE_ENGINE> <CUSTOM_TABLE_OPTIONS>;
- SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED;
- START TRANSACTION WITH CONSISTENT SNAPSHOT;
-+Warnings:
-+Warning	138	InnoDB: WITH CONSISTENT SNAPSHOT was ignored because this phrase can only be used with REPEATABLE READ isolation level.
- connection con2;
- INSERT INTO t1 (a) VALUES (1);
- connection con1;

storage/innobase/mysql-test/storage_engine/trx/level_read_uncommitted.rdiff

@@ -5,7 +5,7 @@
  SET SESSION TRANSACTION ISOLATION LEVEL READ UNCOMMITTED;
  START TRANSACTION WITH CONSISTENT SNAPSHOT;
 +Warnings:
-+Warning	138	InnoDB: WITH CONSISTENT SNAPSHOT was ignored because this phrase can only be used with REPEATABLE READ isolation level.
++Warning	138	InnoDB: WITH CONSISTENT SNAPSHOT is ignored at READ UNCOMMITTED isolation level.
  connection con2;
  INSERT INTO t1 (a) VALUES (1);
  connection con1;