From d7fc975cfe3326c7cdbc188250a8a0bed9d9968d Mon Sep 17 00:00:00 2001 From: Vlad Lesin Date: Tue, 2 Apr 2024 20:45:29 +0300 Subject: [PATCH] MDEV-33802 Weird read view after ROLLBACK of other transactions. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the case if some unique key fields are nullable, there can be several records with the same key fields in unique index with at least one key field equal to NULL, as NULL != NULL. When transaction is resumed after waiting on the record with at least one key field equal to NULL, and stored in persistent cursor record is deleted, persistent cursor can be restored to the record with all key fields equal to the stored ones, but with at least one field equal to NULL. And such record is wrongly treated as a record with the same unique key as stored in persistent cursor record one, what is wrong as NULL != NULL. The fix is to check if at least one unique field is NULL in restored persistent cursor position, and, if so, then don't treat the record as one with the same unique key as in the stored record key. dict_index_t::nulls_equal was removed, as it was initially developed for never existed in MariaDB "intrinsic tables", and there is no code, which would set it to "true". Reviewed by Marko Mäkelä. --- .../r/cursor-restore-unique-null.result | 24 +++++++++++++ .../innodb/t/cursor-restore-unique-null.test | 36 +++++++++++++++++++ storage/innobase/btr/btr0pcur.cc | 11 ++++-- storage/innobase/dict/dict0dict.cc | 1 - storage/innobase/include/data0data.h | 13 +++---- storage/innobase/include/data0data.inl | 30 ++++++---------- storage/innobase/include/dict0mem.h | 2 -- storage/innobase/include/dict0mem.inl | 1 - storage/innobase/row/row0ins.cc | 14 ++------ 9 files changed, 87 insertions(+), 45 deletions(-) create mode 100644 mysql-test/suite/innodb/r/cursor-restore-unique-null.result create mode 100644 mysql-test/suite/innodb/t/cursor-restore-unique-null.test diff --git a/mysql-test/suite/innodb/r/cursor-restore-unique-null.result b/mysql-test/suite/innodb/r/cursor-restore-unique-null.result new file mode 100644 index 00000000000..b5e2de88e7a --- /dev/null +++ b/mysql-test/suite/innodb/r/cursor-restore-unique-null.result @@ -0,0 +1,24 @@ +CREATE TABLE t(a INT PRIMARY KEY, b INT, c INT, UNIQUE KEY `b_c` (`b`,`c`)) +ENGINE=InnoDB, STATS_PERSISTENT=0; +INSERT INTO t SET a = 1, c = 2; +connect con1,localhost,root; +BEGIN; +INSERT INTO t SET a=2, c=2; +connection default; +BEGIN; +SET DEBUG_SYNC="lock_wait_suspend_thread_enter SIGNAL select_locked"; +SELECT * FROM t FORCE INDEX(b) FOR UPDATE; +connection con1; +SET DEBUG_SYNC="now WAIT_FOR select_locked"; +ROLLBACK; +connection default; +# If the bug is not fixed, and the both unique index key fields are +# NULL, there will be two (1, NULL, 2) rows in the result, +# because cursor will be restored to (NULL, 2, 1) position for +# secondary key instead of "supremum". +a b c +1 NULL 2 +COMMIT; +SET DEBUG_SYNC="RESET"; +disconnect con1; +DROP TABLE t; diff --git a/mysql-test/suite/innodb/t/cursor-restore-unique-null.test b/mysql-test/suite/innodb/t/cursor-restore-unique-null.test new file mode 100644 index 00000000000..c22ddf89d8f --- /dev/null +++ b/mysql-test/suite/innodb/t/cursor-restore-unique-null.test @@ -0,0 +1,36 @@ +--source include/have_innodb.inc +--source include/have_debug.inc +--source include/have_debug_sync.inc +--source include/count_sessions.inc + + +CREATE TABLE t(a INT PRIMARY KEY, b INT, c INT, UNIQUE KEY `b_c` (`b`,`c`)) + ENGINE=InnoDB, STATS_PERSISTENT=0; +INSERT INTO t SET a = 1, c = 2; + +--connect con1,localhost,root +BEGIN; + INSERT INTO t SET a=2, c=2; + +--connection default +BEGIN; +SET DEBUG_SYNC="lock_wait_suspend_thread_enter SIGNAL select_locked"; +--send SELECT * FROM t FORCE INDEX(b) FOR UPDATE + +--connection con1 +SET DEBUG_SYNC="now WAIT_FOR select_locked"; +ROLLBACK; + +--connection default +--echo # If the bug is not fixed, and the both unique index key fields are +--echo # NULL, there will be two (1, NULL, 2) rows in the result, +--echo # because cursor will be restored to (NULL, 2, 1) position for +--echo # secondary key instead of "supremum". +--reap +COMMIT; + +SET DEBUG_SYNC="RESET"; + +--disconnect con1 +DROP TABLE t; +--source include/wait_until_count_sessions.inc diff --git a/storage/innobase/btr/btr0pcur.cc b/storage/innobase/btr/btr0pcur.cc index 90084ef4379..4296d94fe5e 100644 --- a/storage/innobase/btr/btr0pcur.cc +++ b/storage/innobase/btr/btr0pcur.cc @@ -453,8 +453,15 @@ btr_pcur_t::restore_position(ulint restore_latch_mode, const char *file, return SAME_ALL; } - if (n_matched_fields >= index->n_uniq) - ret_val= SAME_UNIQ; + if (n_matched_fields >= index->n_uniq + /* Unique indexes can contain "NULL" keys, and if all + unique fields are NULL and not all tuple + fields match to record fields, then treat it as if + restored cursor position points to the record with + not the same unique key. */ + && !(index->n_nullable + && dtuple_contains_null(tuple, index->n_uniq))) + ret_val= SAME_UNIQ; } mem_heap_free(heap); diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc index d0f8ac21dd8..26671d85b3d 100644 --- a/storage/innobase/dict/dict0dict.cc +++ b/storage/innobase/dict/dict0dict.cc @@ -1849,7 +1849,6 @@ dict_index_add_to_cache( new_index->n_fields = new_index->n_def; new_index->trx_id = index->trx_id; new_index->set_committed(index->is_committed()); - new_index->nulls_equal = index->nulls_equal; #ifdef MYSQL_INDEX_DISABLE_AHI new_index->disable_ahi = index->disable_ahi; #endif diff --git a/storage/innobase/include/data0data.h b/storage/innobase/include/data0data.h index c2b8c3e00b6..a2c39ecaed8 100644 --- a/storage/innobase/include/data0data.h +++ b/storage/innobase/include/data0data.h @@ -349,15 +349,12 @@ dtuple_set_types_binary( dtuple_t* tuple, /*!< in: data tuple */ ulint n) /*!< in: number of fields to set */ MY_ATTRIBUTE((nonnull)); -/**********************************************************************//** -Checks if a dtuple contains an SQL null value. -@return TRUE if some field is SQL null */ +/** Checks if a dtuple contains an SQL null value. +@param tuple tuple +@param fields_number number of fields in the tuple to check +@return true if some field is SQL null */ UNIV_INLINE -ibool -dtuple_contains_null( -/*=================*/ - const dtuple_t* tuple) /*!< in: dtuple */ - MY_ATTRIBUTE((nonnull, warn_unused_result)); +bool dtuple_contains_null(const dtuple_t *tuple, ulint fields_number = 0); /**********************************************************//** Checks that a data field is typed. Asserts an error if not. @return TRUE if ok */ diff --git a/storage/innobase/include/data0data.inl b/storage/innobase/include/data0data.inl index 39ade7b1e09..f4ae7528d3f 100644 --- a/storage/innobase/include/data0data.inl +++ b/storage/innobase/include/data0data.inl @@ -599,28 +599,18 @@ data_write_sql_null( memset(data, 0, len); } -/**********************************************************************//** -Checks if a dtuple contains an SQL null value. -@return TRUE if some field is SQL null */ +/** Checks if a dtuple contains an SQL null value. +@param tuple tuple +@param fields_number number of fields in the tuple to check +@return true if some field is SQL null */ UNIV_INLINE -ibool -dtuple_contains_null( -/*=================*/ - const dtuple_t* tuple) /*!< in: dtuple */ +bool dtuple_contains_null(const dtuple_t *tuple, ulint fields_number) { - ulint n; - ulint i; - - n = dtuple_get_n_fields(tuple); - - for (i = 0; i < n; i++) { - if (dfield_is_null(dtuple_get_nth_field(tuple, i))) { - - return(TRUE); - } - } - - return(FALSE); + ulint n= fields_number ? fields_number : dtuple_get_n_fields(tuple); + for (ulint i= 0; i < n; i++) + if (dfield_is_null(dtuple_get_nth_field(tuple, i))) + return true; + return false; } /**************************************************************//** diff --git a/storage/innobase/include/dict0mem.h b/storage/innobase/include/dict0mem.h index f0d4eb7bebf..d81bbb51b43 100644 --- a/storage/innobase/include/dict0mem.h +++ b/storage/innobase/include/dict0mem.h @@ -1011,8 +1011,6 @@ struct dict_index_t { /*!< number of columns the user defined to be in the index: in the internal representation we add more columns */ - unsigned nulls_equal:1; - /*!< if true, SQL NULL == SQL NULL */ #ifdef BTR_CUR_HASH_ADAPT #ifdef MYSQL_INDEX_DISABLE_AHI unsigned disable_ahi:1; diff --git a/storage/innobase/include/dict0mem.inl b/storage/innobase/include/dict0mem.inl index 090ec73278b..28900c9062f 100644 --- a/storage/innobase/include/dict0mem.inl +++ b/storage/innobase/include/dict0mem.inl @@ -63,7 +63,6 @@ dict_mem_fill_index_struct( index->n_core_fields = (unsigned int) n_fields; /* The '1 +' above prevents allocation of an empty mem block */ - index->nulls_equal = false; #ifdef BTR_CUR_HASH_ADAPT #ifdef MYSQL_INDEX_DISABLE_AHI index->disable_ahi = false; diff --git a/storage/innobase/row/row0ins.cc b/storage/innobase/row/row0ins.cc index 088981e5b5b..6e68f46825a 100644 --- a/storage/innobase/row/row0ins.cc +++ b/storage/innobase/row/row0ins.cc @@ -2038,7 +2038,7 @@ row_ins_dupl_error_with_rec( /* In a unique secondary index we allow equal key values if they contain SQL NULLs */ - if (!dict_index_is_clust(index) && !index->nulls_equal) { + if (!dict_index_is_clust(index)) { for (i = 0; i < n_unique; i++) { if (dfield_is_null(dtuple_get_nth_field(entry, i))) { @@ -2143,16 +2143,8 @@ row_ins_scan_sec_index_for_duplicate( /* If the secondary index is unique, but one of the fields in the n_unique first fields is NULL, a unique key violation cannot occur, since we define NULL != NULL in this case */ - - if (!index->nulls_equal) { - for (ulint i = 0; i < n_unique; i++) { - if (UNIV_SQL_NULL == dfield_get_len( - dtuple_get_nth_field(entry, i))) { - - DBUG_RETURN(DB_SUCCESS); - } - } - } + if (index->n_nullable && dtuple_contains_null(entry, n_unique)) + DBUG_RETURN(DB_SUCCESS); /* Store old value on n_fields_cmp */