Bug #99759 Three cases QueryCache hit query result error cause result mismatch.
Submitted: 2 Jun 2020 12:23 Modified: 4 Jun 2020 3:36
Reporter: Ze Yang Email Updates:
Status: Verified Impact on me:
None 
Category:MySQL Server: Query Cache Severity:S3 (Non-critical)
Version:5.6, 5.7 OS:Any
Assigned to: CPU Architecture:Any

[2 Jun 2020 12:23] Ze Yang
Description:
For InnoDB engine, when try to hit query cache, need to call row_search_check_if_query_cache_permitted.

This function use trx_id to check whether the cache result can be saw by current transaction.

```
6426 ibool
6427 row_search_check_if_query_cache_permitted(
6428 /*======================================*/
6429   trx_t*    trx,    /*!< in: transaction object */
6430   const char* norm_name)  /*!< in: concatenation of database name,
6431           '/' char, table name */
6432 {
6433   dict_table_t* table;
6434   ibool   ret = FALSE;
6435
6436   table = dict_table_open_on_name(
6437     norm_name, FALSE, FALSE, DICT_ERR_IGNORE_NONE);
6438
6439   if (table == NULL) {
6440
6441     return(FALSE);
6442   }
6443
6444   /* Start the transaction if it is not started yet */
6445
6446   trx_start_if_not_started(trx, false);
6447
6448   /* If there are locks on the table or some trx has invalidated the
6449   cache before this transaction started then this transaction cannot
6450   read/write from/to the cache.
6451
6452   If a read view has not been created for the transaction then it doesn't
6453   really matter what this transactin sees. If a read view was created
6454   then the view low_limit_id is the max trx id that this transaction
6455   saw at the time of the read view creation.  */
6456
6457   if (lock_table_get_n_locks(table) == 0
6458       && ((trx->id != 0 && trx->id >= table->query_cache_inv_id)
6459     || !MVCC::is_view_active(trx->read_view)
6460     || trx->read_view->low_limit_id()
6461     >= table->query_cache_inv_id)) {
```

But this is wrong. We can not just use trx_id comparison with  query_cache_inv_id which is the last committed trx id to check the visibility.  Because the trx_id is the start of rw-transaction, may be when current transaction  start, the rw-trx has not been committed.

So, the conditions
 
```
((trx->id != 0 && trx->id >= table->query_cache_inv_id)
```

and 

```
6459     || !MVCC::is_view_active(trx->read_view)
6460     || trx->read_view->low_limit_id()
6461     >= table->query_cache_inv_id)
```

can not guarantee result right.

Another is the dict table may be remove from cache. The table->query_cache_inv_id may be 0 after dict_table_open_on_name. This also may cause result mismatch.

How to repeat:

query_cache_bugs-master.opt
```
--query_cache_type=1
```

query_cache_bugs.test
```
set GLOBAL query_cache_size=1355776;
CREATE TABLE t1(c1 INT);
CREATE TABLE t2(c1 INT);
SET @@GLOBAL.innodb_stats_auto_recalc=OFF;
connect (con1, localhost, root);
connect (con2, localhost, root);

--echo Case1. ReadView low_limit_id check with query_cache_inv_id can not guarantee result correct.
--enable_connect_log
connection con1;
SET TRANSACTION_ISOLATION='REPEATABLE-READ';
BEGIN;
INSERT INTO t2 VALUES(1);

connection default;
SET TRANSACTION_ISOLATION='REPEATABLE-READ';
BEGIN;
SELECT * FROM t1;

connection con1;
INSERT INTO t1 VALUES(1);
COMMIT;

connection con2;
SET TRANSACTION_ISOLATION='REPEATABLE-READ';
BEGIN;
SELECT * FROM t1;

connection default;
SELECT * FROM t1;
SELECT * FROM t1 WHERE 1;
COMMIT;

connection con2;
COMMIT;

RESET QUERY CACHE;
--echo Case2. Trx id check with query_cache_inv_id can not guarantee result correct.
connection default;
BEGIN;
SELECT * FROM t1;

connection con1;
BEGIN;
INSERT INTO t1 VALUES(2);
COMMIT;

connection con2;
SET TRANSACTION_ISOLATION='REPEATABLE-READ';
BEGIN;
SELECT * FROM t1;

connection default;
#Change to write trx. trx_id is allocated after INSERT.
INSERT INTO t2 VALUES(2);
SELECT * FROM t1;
SELECT * FROM t1 WHERE 1;
COMMIT;

connection con2;
COMMIT;

RESET QUERY CACHE;
#--echo Case3. dict_table remove from cache. The check with query_cache_inv_id can not guarantee result correct.
#set @@global.table_definition_cache=400;
#--let $table_count=405
#--let $table_number=1
#while ($table_number <= $table_count)
#{
#  --eval CREATE TABLE tb$table_number (c1 int);
#  --inc $table_number
#}
#
#connection default;
#BEGIN;
#SELECT * FROM t2;
#
#connection con1;
#BEGIN;
#INSERT INTO t1 VALUES(10);
#COMMIT;
#
#connection con2;
#BEGIN;
#SELECT * FROM t1;
#COMMIT;
#
#BEGIN;
#--let $table_number=1
#while ($table_number <= $table_count)
#{
#  --eval SELECT * FROM tb$table_number;
#  --inc $table_number
#}
#
#connection default;
#disconnect con1;
#disconnect con2;
##Wait dict_table removed from cache
#select sleep(5);
#SELECT * FROM t1;
#SELECT * FROM t1 WHERE 1;
#COMMIT;
#
#connection con2;
#COMMIT;

connection default;
disconnect con1;
disconnect con2;
DROP TABLE t1, t2;
```

query_cache_bugs.result
```
set GLOBAL query_cache_size=1355776;
Warnings:
Warning 1287  '@@query_cache_size' is deprecated and will be removed in a future release.
CREATE TABLE t1(c1 INT);
CREATE TABLE t2(c1 INT);
SET @@GLOBAL.innodb_stats_auto_recalc=OFF;
Case1. ReadView low_limit_id check with query_cache_inv_id can not guarantee result correct.
connection con1;
SET TRANSACTION_ISOLATION='REPEATABLE-READ';
BEGIN;
INSERT INTO t2 VALUES(1);
connection default;
SET TRANSACTION_ISOLATION='REPEATABLE-READ';
BEGIN;
SELECT * FROM t1;
c1
connection con1;
INSERT INTO t1 VALUES(1);
COMMIT;
connection con2;
SET TRANSACTION_ISOLATION='REPEATABLE-READ';
BEGIN;
SELECT * FROM t1;
c1
1
connection default;
SELECT * FROM t1;         --result mismatch
c1
1
SELECT * FROM t1 WHERE 1;
c1
COMMIT;
connection con2;
COMMIT;
RESET QUERY CACHE;
Warnings:
Warning 1681  'RESET QUERY CACHE' is deprecated and will be removed in a future release.
Case2. Trx id check with query_cache_inv_id can not guarantee result correct.
connection default;
BEGIN;
SELECT * FROM t1;
c1
1
connection con1;
BEGIN;
INSERT INTO t1 VALUES(2);
COMMIT;
connection con2;
SET TRANSACTION_ISOLATION='REPEATABLE-READ';
BEGIN;
SELECT * FROM t1;
c1
1
2
connection default;
INSERT INTO t2 VALUES(2);
SELECT * FROM t1;                          --result mismatch
c1
1
2
SELECT * FROM t1 WHERE 1;
c1
1
COMMIT;
connection con2;
COMMIT;
RESET QUERY CACHE;
Warnings:
Warning 1681  'RESET QUERY CACHE' is deprecated and will be removed in a future release.
connection default;
disconnect con1;
disconnect con2;
DROP TABLE t1, t2;
```

As I can not make the dict_table may remove from cache in mtr test happen. (Which could happen in independent server), so I disable case3. The reason why dict_table t1 not remove from cache is like below. Owned by thread id 0.
```
mysql> select * from table_handles;
+-------------+---------------+-------------+-----------------------+-----------------+----------------+---------------+---------------+
| OBJECT_TYPE | OBJECT_SCHEMA | OBJECT_NAME | OBJECT_INSTANCE_BEGIN | OWNER_THREAD_ID | OWNER_EVENT_ID | INTERNAL_LOCK | EXTERNAL_LOCK |
+-------------+---------------+-------------+-----------------------+-----------------+----------------+---------------+---------------+
| TABLE       | sys           | sys_config  |       139862459739728 |               0 |              0 | NULL          | NULL          |
| TABLE       | test          | t2          |       139862254616416 |               0 |              0 | NULL          | NULL          |
| TABLE       | test          | t1          |       139862469611856 |               0 |              0 | NULL          | NULL          |
| TABLE       | test          | t1          |       139862254624512 |               0 |              0 | NULL
```

Suggested fix:

Use committed version which is changed when commit trx.
And load new value of trx_sys_get_committed_version when query_cache_inv_id is 0.

diff --git a/storage/innobase/include/read0types.h b/storage/innobase/include/read0types.h
index 49967f289f6..002b66036e8 100644
--- a/storage/innobase/include/read0types.h
+++ b/storage/innobase/include/read0types.h
@@ -249,6 +249,8 @@ public:
                return(m_ids.empty());
        }

+  ib_id_t get_committed_version() { return m_committed_version; }
+
 #ifdef UNIV_DEBUG
        /**
        @param rhs              view to compare with
@@ -326,6 +328,8 @@ private:
        they can be removed in purge if not needed by other views */
        trx_id_t        m_low_limit_no;

+  ib_id_t m_committed_version;
+
        /** AC-NL-RO transaction view that has been "closed". */
        bool            m_closed;

diff --git a/storage/innobase/include/trx0sys.h b/storage/innobase/include/trx0sys.h
index 5b82a9019f1..44e23158349 100644
--- a/storage/innobase/include/trx0sys.h
+++ b/storage/innobase/include/trx0sys.h
@@ -173,6 +173,8 @@ UNIV_INLINE
 trx_id_t
 trx_sys_get_max_trx_id(void);
 /*========================*/
+UNIV_INLINE
+ib_id_t trx_sys_get_committed_version(void);

 #ifdef UNIV_DEBUG
 /* Flag to control TRX_RSEG_N_SLOTS behavior debugging. */
@@ -579,6 +581,8 @@ struct trx_sys_t {
                                        volatile because it can be accessed
                                        without holding any mutex during
                                        AC-NL-RO view creation. */
+  volatile ib_uint64_t committed_version;
+
        trx_ut_list_t   serialisation_list;
                                        /*!< Ordered on trx_t::no of all the
                                        currenrtly active RW transactions */
diff --git a/storage/innobase/include/trx0sys.ic b/storage/innobase/include/trx0sys.ic
index 4f3642b4719..69783945023 100644
--- a/storage/innobase/include/trx0sys.ic
+++ b/storage/innobase/include/trx0sys.ic
@@ -474,6 +474,10 @@ trx_sys_get_max_trx_id(void)
 #endif /* UNIV_WORD_SIZE < DATA_TRX_ID_LEN */
 }

+inline ib_id_t trx_sys_get_committed_version() {
+  return(trx_sys->committed_version);
+}
+
 /*****************************************************************//**
 Get the number of transaction in the system, independent of their state.
 @return count of transactions in trx_sys_t::rw_trx_list */
diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc
index 1f46f944e08..dadeb103465 100644
--- a/storage/innobase/lock/lock0lock.cc
+++ b/storage/innobase/lock/lock0lock.cc
@@ -4473,7 +4473,6 @@ lock_release(
 {
        lock_t*         lock;
        ulint           count = 0;
-       trx_id_t        max_trx_id = trx_sys_get_max_trx_id();

        ut_ad(lock_mutex_own());
        ut_ad(!trx_mutex_own(trx));
@@ -4500,7 +4499,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_id = trx_sys_get_committed_version();
                        }

                        lock_table_dequeue(lock);
diff --git a/storage/innobase/read/read0read.cc b/storage/innobase/read/read0read.cc
index 305fd5dc381..8ca6ad646c0 100644
--- a/storage/innobase/read/read0read.cc
+++ b/storage/innobase/read/read0read.cc
@@ -333,7 +333,8 @@ ReadView::ReadView()
        m_up_limit_id(),
        m_creator_trx_id(),
        m_ids(),
-       m_low_limit_no()
+       m_low_limit_no(),
+  m_committed_version()
 {
        ut_d(::memset(&m_view_list, 0x0, sizeof(m_view_list)));
 }
@@ -473,6 +474,7 @@ ReadView::prepare(trx_id_t id)
                        m_low_limit_no = trx->no;
                }
        }
+  m_committed_version = trx_sys->committed_version;
 }

 /**
diff --git a/storage/innobase/row/row0sel.cc b/storage/innobase/row/row0sel.cc
index c37e7908bc5..bc15f7d80d8 100644
--- a/storage/innobase/row/row0sel.cc
+++ b/storage/innobase/row/row0sel.cc
@@ -6441,6 +6441,8 @@ row_search_check_if_query_cache_permitted(
                return(FALSE);
        }

+  if (table->query_cache_inv_id == 0)
+    table->query_cache_inv_id = trx_sys_get_committed_version();
        /* Start the transaction if it is not started yet */

        trx_start_if_not_started(trx, false);
@@ -6455,9 +6457,8 @@ row_search_check_if_query_cache_permitted(
        saw at the time of the read view creation.  */

        if (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()
+           && (!MVCC::is_view_active(trx->read_view)
+               || trx->read_view->get_committed_version()
                >= table->query_cache_inv_id)) {

                ret = TRUE;
diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc
index 30885ae02a0..edc4fb9f116 100644
--- a/storage/innobase/trx/trx0trx.cc
+++ b/storage/innobase/trx/trx0trx.cc
@@ -1905,6 +1905,7 @@ trx_erase_lists(
        }

        trx_sys->rw_trx_set.erase(TrxTrack(trx->id));
+  trx_sys->committed_version++;

        trx_sys_mutex_exit();
 }

diff --git a/storage/innobase/trx/trx0sys.cc b/storage/innobase/trx/trx0sys.cc
index 1e553d91373..825451bc60d 100644
--- a/storage/innobase/trx/trx0sys.cc
+++ b/storage/innobase/trx/trx0sys.cc
@@ -533,6 +533,7 @@ trx_sys_init_at_db_start(void)

                ib::info() << "Trx id counter is " << trx_sys->max_trx_id;
        }
+  trx_sys->committed_version = 0;

        trx_sys_mutex_exit();
[3 Jun 2020 12:50] MySQL Verification Team
Hi Mr. Yang,

Thank you for your bug report.

We must say that this is very well detailed and convincing bug report.

Unfortunately, query cache is totally deprecated and not maintained, even in the versions that you have cited.

However, I find your code review correct and hence I am verifying it. There are other teams that will decide on whether this will be fixed or not.

Verified as reported.
[3 Jun 2020 13:00] MySQL Verification Team
Setting correct status.
[4 Jun 2020 3:36] Ze Yang
I know that query cache is totally deprecated in 8.0 and not maintained.

Even the bugs would not been fixed, I report the bugs may help others who want to use query cache or meet the error.
[4 Jun 2020 12:24] MySQL Verification Team
Thank you, Mr. Yang.