MDEV-16087 Inconsistent SELECT results when query cache is enabled

The following conditions will decide the query cache retrieval or
storing inside innodb:

    (1) There should not be any locks on the table.
    (2) Some other trx shouldn't invalidated the cache before the
    transaction started.
    (3) Read view shouldn't exist. If exists then the view
    low_limit_id should be greater than or equal to the transaction that
    invalidates the cache for the particular table.

 For read-only transaction: should satisfy  the above (1) and (3)
 For read-write transaction: should satisfy the above (1), (2), (3).

 - Changed the variable from query_cache_inv_id to query_cache_inv_trx_id.

 - Moved the function row_search_check_if_query_cache_permitted from
    row0sel.h and made it as static function in ha_innodb.cc

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

@@ -0,0 +1,39 @@
+#
+# MDEV-16087 Inconsistent SELECT results when query cache is enabled
+#
+set GLOBAL query_cache_type=ON;
+set LOCAL query_cache_type=ON;
+create table t1 (id bigint(20) auto_increment, primary key (id)) ENGINE=InnoDB;
+create table t2 (id bigint(20) auto_increment, primary key (id)) ENGINE=InnoDB;
+create table t3 (id bigint(20) auto_increment, primary key (id)) ENGINE=InnoDB;
+connect  con1,localhost,root;
+START TRANSACTION WITH CONSISTENT SNAPSHOT;
+select * from t2;
+id
+connection default;
+insert into t3 () values ();
+connection con1;
+insert into t1 () values ();
+select * from t3;
+id
+connect  con2,localhost,root;
+START TRANSACTION WITH CONSISTENT SNAPSHOT;
+select * from t3;
+id
+1
+select * from t3;
+id
+1
+select sql_no_cache * from t3;
+id
+1
+rollback;
+connection con1;
+rollback;
+disconnect con1;
+disconnect con2;
+connection default;
+drop table t1;
+drop table t2;
+drop table t3;
+set GLOBAL query_cache_type=default;

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

@@ -0,0 +1,47 @@
+-- source include/have_innodb.inc
+-- source include/have_query_cache.inc
+-- source include/not_embedded.inc
+
+--echo #
+--echo # MDEV-16087 Inconsistent SELECT results when query cache is enabled
+--echo #
+
+set GLOBAL query_cache_type=ON;
+set LOCAL query_cache_type=ON;
+
+create table t1 (id bigint(20) auto_increment, primary key (id)) ENGINE=InnoDB;
+create table t2 (id bigint(20) auto_increment, primary key (id)) ENGINE=InnoDB;
+create table t3 (id bigint(20) auto_increment, primary key (id)) ENGINE=InnoDB;
+
+connect (con1,localhost,root);
+START TRANSACTION WITH CONSISTENT SNAPSHOT;
+select * from t2;
+
+connection default;
+insert into t3 () values ();
+
+connection con1;
+insert into t1 () values ();
+select * from t3;
+
+connect (con2,localhost,root);
+START TRANSACTION WITH CONSISTENT SNAPSHOT;
+select * from t3;
+select * from t3;
+select sql_no_cache * from t3;
+
+rollback;
+
+connection con1;
+
+rollback;
+
+disconnect con1;
+disconnect con2;
+connection default;
+
+drop table t1;
+drop table t2;
+drop table t3;
+
+set GLOBAL query_cache_type=default;

storage/innobase/handler/ha_innodb.cc

@@ -3167,6 +3167,83 @@ AUTOCOMMIT==0 or we are inside BEGIN ... COMMIT. Thus transactions no longer
 put restrictions on the use of the query cache.
 */
 
+/** Check if mysql can allow the transaction to read from/store to
+the query cache.
+@param[in]	table	table object
+@param[in]	trx	transaction object
+@return whether the storing or retrieving from the query cache is permitted */
+static bool innobase_query_caching_table_check_low(
+	const dict_table_t*	table,
+	trx_t*			trx)
+{
+	/* The following conditions will decide the query cache
+	retrieval or storing into:
+
+	(1) There should not be any locks on the table.
+	(2) Someother trx shouldn't invalidate the cache before this
+	transaction started.
+	(3) Read view shouldn't exist. If exists then the view
+	low_limit_id should be greater than or equal to the transaction that
+	invalidates the cache for the particular table.
+
+	For read-only transaction: should satisfy (1) and (3)
+	For read-write transaction: should satisfy (1), (2), (3) */
+
+	if (lock_table_get_n_locks(table)) {
+		return false;
+	}
+
+	if (trx->id && trx->id < table->query_cache_inv_trx_id) {
+		return false;
+	}
+
+	return !MVCC::is_view_active(trx->read_view)
+		|| trx->read_view->low_limit_id()
+		>= table->query_cache_inv_trx_id;
+}
+
+/** Checks if MySQL at the moment is allowed for this table to retrieve a
+consistent read result, or store it to the query cache.
+@param[in,out]	trx		transaction
+@param[in]	norm_name	concatenation of database name,
+				'/' char, table name
+@return whether storing or retrieving from the query cache is permitted */
+static bool innobase_query_caching_table_check(
+	trx_t*		trx,
+	const char*	norm_name)
+{
+	dict_table_t*   table = dict_table_open_on_name(
+		norm_name, FALSE, FALSE, DICT_ERR_IGNORE_NONE);
+
+	if (table == NULL) {
+		return false;
+	}
+
+	/* Start the transaction if it is not started yet */
+	trx_start_if_not_started(trx, false);
+
+	bool allow = innobase_query_caching_table_check_low(table, trx);
+
+	dict_table_close(table, FALSE, FALSE);
+
+	if (allow) {
+		/* If the isolation level is high, assign a read view for the
+		transaction if it does not yet have one */
+
+		if (trx->isolation_level >= TRX_ISO_REPEATABLE_READ
+		    && !srv_read_only_mode
+		    && !MVCC::is_view_active(trx->read_view)) {
+
+			/* Start the transaction if it is not started yet */
+			trx_start_if_not_started(trx, false);
+
+			trx_sys->mvcc->view_open(trx->read_view, trx);
+		}
+	}
+
+	return allow;
+}
+
 /******************************************************************//**
 The MySQL query cache uses this to check from InnoDB if the query cache at
 the moment is allowed to operate on an InnoDB table. The SQL query must
@@ -3242,7 +3319,7 @@ innobase_query_caching_of_table_permitted(
 
 	innobase_register_trx(innodb_hton_ptr, thd, trx);
 
-	return(row_search_check_if_query_cache_permitted(trx, norm_name));
+	return innobase_query_caching_table_check(trx, norm_name);
 }
 
 /*****************************************************************//**

storage/innobase/include/dict0mem.h

@@ -1546,8 +1546,8 @@ struct dict_table_t {
 	/** Transactions whose view low limit is greater than this number are
 	not allowed to store to the MySQL query cache or retrieve from it.
 	When a trx with undo logs commits, it sets this to the value of the
-	current time. */
-	trx_id_t				query_cache_inv_id;
+	transaction id. */
+	trx_id_t				query_cache_inv_trx_id;
 
 	/** Transaction id that last touched the table definition. Either when
 	loading the definition or CREATE TABLE, or ALTER TABLE (prepare,

storage/innobase/include/row0sel.h

@@ -213,16 +213,6 @@ row_count_rtree_recs(
 	ulint*		n_rows);	/*!< out: number of entries
 					seen in the consistent read */
 
-/*******************************************************************//**
-Checks if MySQL at the moment is allowed for this table to retrieve a
-consistent read result, or store it to the query cache.
-@return whether storing or retrieving from the query cache is permitted */
-bool
-row_search_check_if_query_cache_permitted(
-/*======================================*/
-	trx_t*		trx,		/*!< in: transaction object */
-	const char*	norm_name);	/*!< in: concatenation of database name,
-					'/' char, table name */
 /** Read the max AUTOINC value from an index.
 @param[in] index	index starting with an AUTO_INCREMENT column
 @return	the largest AUTO_INCREMENT value

storage/innobase/lock/lock0lock.cc

@@ -4456,7 +4456,7 @@ lock_release(
 				block the use of the MySQL query cache for
 				all currently active transactions. */
 
-				table->query_cache_inv_id = max_trx_id;
+				table->query_cache_inv_trx_id = max_trx_id;
 			}
 
 			lock_table_dequeue(lock);

storage/innobase/row/row0sel.cc

@@ -5905,60 +5905,6 @@ row_count_rtree_recs(
 	goto loop;
 }
 
-/*******************************************************************//**
-Checks if MySQL at the moment is allowed for this table to retrieve a
-consistent read result, or store it to the query cache.
-@return whether storing or retrieving from the query cache is permitted */
-bool
-row_search_check_if_query_cache_permitted(
-/*======================================*/
-	trx_t*		trx,		/*!< in: transaction object */
-	const char*	norm_name)	/*!< in: concatenation of database name,
-					'/' char, table name */
-{
-	dict_table_t*	table = dict_table_open_on_name(
-		norm_name, FALSE, FALSE, DICT_ERR_IGNORE_NONE);
-
-	if (table == NULL) {
-
-		return(false);
-	}
-
-	/* Start the transaction if it is not started yet */
-
-	trx_start_if_not_started(trx, false);
-
-	/* If there are locks on the table or some trx has invalidated the
-	cache before this transaction started then this transaction cannot
-	read/write from/to the cache.
-
-	If a read view has not been created for the transaction then it doesn't
-	really matter what this transaction sees. If a read view was created
-	then the view low_limit_id is the max trx id that this transaction
-	saw at the time of the read view creation.  */
-
-	const bool ret = lock_table_get_n_locks(table) == 0
-		&& ((trx->id != 0 && trx->id >= table->query_cache_inv_id)
-		    || !MVCC::is_view_active(trx->read_view)
-		    || trx->read_view->low_limit_id()
-		    >= table->query_cache_inv_id);
-	if (ret) {
-		/* If the isolation level is high, assign a read view for the
-		transaction if it does not yet have one */
-
-		if (trx->isolation_level >= TRX_ISO_REPEATABLE_READ
-		    && !srv_read_only_mode
-		    && !MVCC::is_view_active(trx->read_view)) {
-
-			trx_sys->mvcc->view_open(trx->read_view, trx);
-		}
-	}
-
-	dict_table_close(table, FALSE, FALSE);
-
-	return(ret);
-}
-
 /*******************************************************************//**
 Read the AUTOINC column from the current row. If the value is less than
 0 and the type is not unsigned then we reset the value to 0.