From a6f258e47f425a3ebecf6aaba87bdfcc241dc416 Mon Sep 17 00:00:00 2001 From: Vlad Lesin Date: Tue, 30 Nov 2021 18:22:39 +0300 Subject: [PATCH] MDEV-20605 Awaken transaction can miss inserted by other transaction records due to wrong persistent cursor restoration Backported from 10.5 20e9e804c131c6522bc7c469e4863e8d1eaa3ee0 and 5948d7602ec7f61937c368dcb134e6ec226a2990. sel_restore_position_for_mysql() moves forward persistent cursor position after btr_pcur_restore_position() call if cursor relative position is BTR_PCUR_ON and the cursor points to the record with NOT the same field values as in a stored record(and some other not important for this case conditions). It was done because btr_pcur_restore_position() sets page_cur_mode_t mode to PAGE_CUR_LE for cursor->rel_pos == BTR_PCUR_ON before opening cursor. So we are searching for the record less or equal to stored one. And if the found record is not equal to stored one, then it is less and we need to move cursor forward. But there can be a situation when the stored record was purged, but the new one with the same key but different value was inserted while row_search_mvcc() was suspended. In this case, when the thread is awaken, it will invoke sel_restore_position_for_mysql(), which, in turns, invoke btr_pcur_restore_position(), which will return false because found record don't match stored record, and sel_restore_position_for_mysql() will move forward cursor position. The above can lead to the case when awaken row_search_mvcc() do not see records inserted by other transactions while it slept. The mtr test case shows the example how it can be. The fix is to return special value from persistent cursor restoring function which would notify its caller that uniq fields of restored record and stored record are the same, and in this case sel_restore_position_for_mysql() don't move cursor forward. Delete-marked records are correctly processed in row_search_mvcc(). Non-unique secondary indexes are "uniquified" by adding the PK, the index->n_uniq should then be index->n_fields. So there is no need in additional checks in the fix. If transaction's readview can't see the changes made in secondary index record, it requests clustered index record in row_search_mvcc() to check its transaction id and get the correspondent record version. After this row_search_mvcc() commits mtr to preserve clustered index latching order, and starts mtr. Between those mtr commit and start secondary index pages are unlatched, and purge has the ability to remove stored in the cursor record, what causes rows duplication in result set for non-locking reads, as cursor position is restored to the previously visited record. To solve this the changes are just switched off for non-locking reads, it's quite simple solution, besides the changes don't make sense for non-locking reads. The more complex and effective from performance perspective solution is to create mtr savepoint before clustered record requesting and rolling back to that savepoint after that. See MDEV-27557. One more solution is to have per-record transaction id for secondary indexes. See MDEV-17598. If any of those is implemented, just remove select_lock_type argument in sel_restore_position_for_mysql(). --- .../innodb/r/cursor-restore-locking.result | 35 ++++ .../innodb/t/cursor-restore-locking.test | 79 ++++++++ storage/innobase/btr/btr0cur.cc | 8 +- storage/innobase/btr/btr0pcur.cc | 187 +++++++++--------- storage/innobase/fts/fts0fts.cc | 11 +- storage/innobase/ibuf/ibuf0ibuf.cc | 3 +- storage/innobase/include/btr0pcur.h | 63 +++--- storage/innobase/row/row0purge.cc | 6 +- storage/innobase/row/row0sel.cc | 69 ++++--- storage/innobase/row/row0uins.cc | 19 +- storage/innobase/row/row0umod.cc | 25 +-- storage/innobase/row/row0upd.cc | 9 +- 12 files changed, 320 insertions(+), 194 deletions(-) create mode 100644 mysql-test/suite/innodb/r/cursor-restore-locking.result create mode 100644 mysql-test/suite/innodb/t/cursor-restore-locking.test diff --git a/mysql-test/suite/innodb/r/cursor-restore-locking.result b/mysql-test/suite/innodb/r/cursor-restore-locking.result new file mode 100644 index 00000000000..bc1127f57b3 --- /dev/null +++ b/mysql-test/suite/innodb/r/cursor-restore-locking.result @@ -0,0 +1,35 @@ +CREATE TABLE t (a int PRIMARY KEY, b int NOT NULL UNIQUE) engine = InnoDB; +connect prevent_purge,localhost,root,,; +start transaction with consistent snapshot; +connect con_del_1,localhost,root,,; +INSERT INTO t VALUES (20,20); +SET DEBUG_SYNC = 'innodb_row_search_for_mysql_exit SIGNAL first_del_row_search_mvcc_finished WAIT_FOR first_del_cont'; +DELETE FROM t WHERE b = 20; +connect con_ins_1,localhost,root,,; +SET DEBUG_SYNC = 'now WAIT_FOR first_del_row_search_mvcc_finished'; +SET DEBUG_SYNC = 'lock_wait_suspend_thread_enter SIGNAL first_ins_locked'; +SET DEBUG_SYNC = 'ib_after_row_insert SIGNAL first_ins_row_inserted WAIT_FOR first_ins_cont'; +INSERT INTO t VALUES(10, 20); +connect con_del_2,localhost,root,,; +SET DEBUG_SYNC = 'now WAIT_FOR first_ins_locked'; +SET DEBUG_SYNC = 'lock_wait_suspend_thread_enter SIGNAL second_del_locked'; +DELETE FROM t WHERE b = 20; +connection default; +SET DEBUG_SYNC = 'now WAIT_FOR second_del_locked'; +SET DEBUG_SYNC = 'now SIGNAL first_del_cont'; +SET DEBUG_SYNC = 'now WAIT_FOR first_ins_row_inserted'; +connection con_del_1; +connection default; +disconnect prevent_purge; +InnoDB 0 transactions not purged +SET DEBUG_SYNC = 'now SIGNAL first_ins_cont'; +connection con_del_2; +connection con_ins_1; +connection default; +INSERT INTO t VALUES(30, 20); +disconnect con_ins_1; +disconnect con_del_1; +disconnect con_del_2; +connection default; +SET DEBUG_SYNC = 'RESET'; +DROP TABLE t; diff --git a/mysql-test/suite/innodb/t/cursor-restore-locking.test b/mysql-test/suite/innodb/t/cursor-restore-locking.test new file mode 100644 index 00000000000..d032d8a8def --- /dev/null +++ b/mysql-test/suite/innodb/t/cursor-restore-locking.test @@ -0,0 +1,79 @@ +--source include/have_innodb.inc +--source include/count_sessions.inc +source include/have_debug.inc; +source include/have_debug_sync.inc; + +CREATE TABLE t (a int PRIMARY KEY, b int NOT NULL UNIQUE) engine = InnoDB; + +--connect(prevent_purge,localhost,root,,) +start transaction with consistent snapshot; + +--connect(con_del_1,localhost,root,,) +INSERT INTO t VALUES (20,20); +SET DEBUG_SYNC = 'innodb_row_search_for_mysql_exit SIGNAL first_del_row_search_mvcc_finished WAIT_FOR first_del_cont'; +--send DELETE FROM t WHERE b = 20 + +--connect(con_ins_1,localhost,root,,) +SET DEBUG_SYNC = 'now WAIT_FOR first_del_row_search_mvcc_finished'; +# It's supposed the following INSERT will be suspended just after +# lock_wait_suspend_thread_enter syncpoint, and will be awaken +# after the previous DELETE commits. ib_after_row_insert will be executed +# after the INSERT is woken up. The previous DELETE will wait for +# first_del_cont signal before commit, and this signal will be sent later. +# So it's safe to use two signals in a row here, it's guaranted the first +# signal will be received before the second signal is sent. +SET DEBUG_SYNC = 'lock_wait_suspend_thread_enter SIGNAL first_ins_locked'; +SET DEBUG_SYNC = 'ib_after_row_insert SIGNAL first_ins_row_inserted WAIT_FOR first_ins_cont'; +--send INSERT INTO t VALUES(10, 20) + +--connect(con_del_2,localhost,root,,) +SET DEBUG_SYNC = 'now WAIT_FOR first_ins_locked'; +SET DEBUG_SYNC = 'lock_wait_suspend_thread_enter SIGNAL second_del_locked'; +############################################################################### +# This DELETE is locked by the previous DELETE, after that DELETE is +# committed, it will still be locked by the next INSERT on delete-marked +# heap_no 2 record. After that INSERT inserted the record with heap_no 3, +# and after heap_no 2 record is purged, this DELETE will be unlocked and +# must restore persistent cursor position at heap_no 3 record, as it has the +# same secondary key value as former heap_no 2 record. Then it must be blocked +# by the previous INSERT, and after the INSERT is committed, it must +# delete the record, inserted by the previous INSERT, and the last INSERT(see +# below) must be finished without error. But instead this DELETE restores +# persistent cursor position to supremum, as a result, it does not delete the +# record, inserted by the previous INSERT, and the last INSERT is finished with +# duplicate key check error. +############################################################################### +--send DELETE FROM t WHERE b = 20 + +--connection default +SET DEBUG_SYNC = 'now WAIT_FOR second_del_locked'; +SET DEBUG_SYNC = 'now SIGNAL first_del_cont'; +SET DEBUG_SYNC = 'now WAIT_FOR first_ins_row_inserted'; +--connection con_del_1 +--reap + +--connection default +--disconnect prevent_purge +--source include/wait_all_purged.inc +SET DEBUG_SYNC = 'now SIGNAL first_ins_cont'; + +--connection con_del_2 +--reap + +--connection con_ins_1 +--reap + +--connection default +############################################################################### +# Duplicate key error is expected if the bug is not fixed. +############################################################################### +INSERT INTO t VALUES(30, 20); + +--disconnect con_ins_1 +--disconnect con_del_1 +--disconnect con_del_2 +--connection default + +SET DEBUG_SYNC = 'RESET'; +DROP TABLE t; +--source include/wait_until_count_sessions.inc diff --git a/storage/innobase/btr/btr0cur.cc b/storage/innobase/btr/btr0cur.cc index 62611c7798a..6e7dd8cfe94 100644 --- a/storage/innobase/btr/btr0cur.cc +++ b/storage/innobase/btr/btr0cur.cc @@ -7213,11 +7213,9 @@ struct btr_blob_log_check_t { buf_block_buf_fix_dec(m_pcur->btr_cur.page_cur.block); } else { ut_ad(m_pcur->rel_pos == BTR_PCUR_ON); - bool ret = btr_pcur_restore_position( - BTR_MODIFY_LEAF | BTR_MODIFY_EXTERNAL, - m_pcur, m_mtr); - - ut_a(ret); + ut_a(btr_pcur_restore_position( + BTR_MODIFY_LEAF | BTR_MODIFY_EXTERNAL, m_pcur, + m_mtr) == btr_pcur_t::SAME_ALL); } *m_block = btr_pcur_get_block(m_pcur); diff --git a/storage/innobase/btr/btr0pcur.cc b/storage/innobase/btr/btr0pcur.cc index ef626e12cf4..53963caa023 100644 --- a/storage/innobase/btr/btr0pcur.cc +++ b/storage/innobase/btr/btr0pcur.cc @@ -256,29 +256,32 @@ struct optimistic_latch_leaves } }; -/**************************************************************//** -Restores the stored position of a persistent cursor bufferfixing the page and -obtaining the specified latches. If the cursor position was saved when the -(1) cursor was positioned on a user record: this function restores the position -to the last record LESS OR EQUAL to the stored record; -(2) cursor was positioned on a page infimum record: restores the position to -the last record LESS than the user record which was the successor of the page -infimum; -(3) cursor was positioned on the page supremum: restores to the first record -GREATER than the user record which was the predecessor of the supremum. -(4) cursor was positioned before the first or after the last in an empty tree: -restores to before first or after the last in the tree. -@return TRUE if the cursor position was stored when it was on a user -record and it can be restored on a user record whose ordering fields -are identical to the ones of the original user record */ -ibool -btr_pcur_restore_position_func( -/*===========================*/ - ulint latch_mode, /*!< in: BTR_SEARCH_LEAF, ... */ - btr_pcur_t* cursor, /*!< in: detached persistent cursor */ - const char* file, /*!< in: file name */ - unsigned line, /*!< in: line where called */ - mtr_t* mtr) /*!< in: mtr */ +/** Restores the stored position of a persistent cursor bufferfixing +the page and obtaining the specified latches. If the cursor position +was saved when the +(1) cursor was positioned on a user record: this function restores the +position to the last record LESS OR EQUAL to the stored record; +(2) cursor was positioned on a page infimum record: restores the +position to the last record LESS than the user record which was the +successor of the page infimum; +(3) cursor was positioned on the page supremum: restores to the first +record GREATER than the user record which was the predecessor of the +supremum. +(4) cursor was positioned before the first or after the last in an +empty tree: restores to before first or after the last in the tree. +@param latch_mode BTR_SEARCH_LEAF, ... +@param file file name +@param line line where called +@param mtr mtr +@return btr_pcur_t::SAME_ALL cursor position on user rec and points on +the record with the same field values as in the stored record, +btr_pcur_t::SAME_UNIQ cursor position is on user rec and points on the +record with the same unique field values as in the stored record, +btr_pcur_t::NOT_SAME cursor position is not on user rec or points on +the record with not the samebuniq field values as in the stored */ +btr_pcur_t::restore_status +btr_pcur_t::restore_position(ulint restore_latch_mode, const char *file, + unsigned line, mtr_t *mtr) { dict_index_t* index; dtuple_t* tuple; @@ -288,104 +291,104 @@ btr_pcur_restore_position_func( ut_ad(mtr->is_active()); //ut_ad(cursor->old_stored); - ut_ad(cursor->pos_state == BTR_PCUR_WAS_POSITIONED - || cursor->pos_state == BTR_PCUR_IS_POSITIONED); + ut_ad(pos_state == BTR_PCUR_WAS_POSITIONED + || pos_state == BTR_PCUR_IS_POSITIONED); - index = btr_cur_get_index(btr_pcur_get_btr_cur(cursor)); + index = btr_cur_get_index(&btr_cur); if (UNIV_UNLIKELY - (cursor->rel_pos == BTR_PCUR_AFTER_LAST_IN_TREE - || cursor->rel_pos == BTR_PCUR_BEFORE_FIRST_IN_TREE)) { + (rel_pos == BTR_PCUR_AFTER_LAST_IN_TREE + || rel_pos == BTR_PCUR_BEFORE_FIRST_IN_TREE)) { dberr_t err = DB_SUCCESS; /* In these cases we do not try an optimistic restoration, but always do a search */ err = btr_cur_open_at_index_side( - cursor->rel_pos == BTR_PCUR_BEFORE_FIRST_IN_TREE, - index, latch_mode, - btr_pcur_get_btr_cur(cursor), 0, mtr); + rel_pos == BTR_PCUR_BEFORE_FIRST_IN_TREE, + index, restore_latch_mode, + &btr_cur, 0, mtr); if (err != DB_SUCCESS) { ib::warn() << " Error code: " << err - << " btr_pcur_restore_position_func " + << " btr_pcur_t::restore_position " << " called from file: " << file << " line: " << line << " table: " << index->table->name << " index: " << index->name; } - cursor->latch_mode = - BTR_LATCH_MODE_WITHOUT_INTENTION(latch_mode); - cursor->pos_state = BTR_PCUR_IS_POSITIONED; - cursor->block_when_stored.clear(); + latch_mode = + BTR_LATCH_MODE_WITHOUT_INTENTION(restore_latch_mode); + pos_state = BTR_PCUR_IS_POSITIONED; + block_when_stored.clear(); - return(FALSE); + return NOT_SAME; } - ut_a(cursor->old_rec); - ut_a(cursor->old_n_core_fields); - ut_a(cursor->old_n_core_fields <= index->n_core_fields); - ut_a(cursor->old_n_fields); + ut_a(old_rec); + ut_a(old_n_core_fields); + ut_a(old_n_core_fields <= index->n_core_fields); + ut_a(old_n_fields); - switch (latch_mode) { + switch (restore_latch_mode) { case BTR_SEARCH_LEAF: case BTR_MODIFY_LEAF: case BTR_SEARCH_PREV: case BTR_MODIFY_PREV: /* Try optimistic restoration. */ - if (cursor->block_when_stored.run_with_hint( - optimistic_latch_leaves(cursor, &latch_mode, + if (block_when_stored.run_with_hint( + optimistic_latch_leaves(this, &restore_latch_mode, mtr))) { - cursor->pos_state = BTR_PCUR_IS_POSITIONED; - cursor->latch_mode = latch_mode; + pos_state = BTR_PCUR_IS_POSITIONED; + latch_mode = restore_latch_mode; buf_block_dbg_add_level( - btr_pcur_get_block(cursor), + btr_pcur_get_block(this), dict_index_is_ibuf(index) ? SYNC_IBUF_TREE_NODE : SYNC_TREE_NODE); - if (cursor->rel_pos == BTR_PCUR_ON) { + if (rel_pos == BTR_PCUR_ON) { #ifdef UNIV_DEBUG const rec_t* rec; rec_offs offsets1_[REC_OFFS_NORMAL_SIZE]; rec_offs offsets2_[REC_OFFS_NORMAL_SIZE]; rec_offs* offsets1 = offsets1_; rec_offs* offsets2 = offsets2_; - rec = btr_pcur_get_rec(cursor); + rec = btr_pcur_get_rec(this); rec_offs_init(offsets1_); rec_offs_init(offsets2_); heap = mem_heap_create(256); - ut_ad(cursor->old_n_core_fields + ut_ad(old_n_core_fields == index->n_core_fields); offsets1 = rec_get_offsets( - cursor->old_rec, index, offsets1, - cursor->old_n_core_fields, - cursor->old_n_fields, &heap); + old_rec, index, offsets1, + old_n_core_fields, + old_n_fields, &heap); offsets2 = rec_get_offsets( rec, index, offsets2, index->n_core_fields, - cursor->old_n_fields, &heap); + old_n_fields, &heap); - ut_ad(!cmp_rec_rec(cursor->old_rec, + ut_ad(!cmp_rec_rec(old_rec, rec, offsets1, offsets2, index)); mem_heap_free(heap); #endif /* UNIV_DEBUG */ - return(TRUE); + return SAME_ALL; } /* This is the same record as stored, may need to be adjusted for BTR_PCUR_BEFORE/AFTER, depending on search mode and direction. */ - if (btr_pcur_is_on_user_rec(cursor)) { - cursor->pos_state + if (btr_pcur_is_on_user_rec(this)) { + pos_state = BTR_PCUR_IS_POSITIONED_OPTIMISTIC; } - return(FALSE); + return NOT_SAME; } } @@ -393,19 +396,19 @@ btr_pcur_restore_position_func( heap = mem_heap_create(256); - tuple = dtuple_create(heap, cursor->old_n_fields); + tuple = dtuple_create(heap, old_n_fields); - dict_index_copy_types(tuple, index, cursor->old_n_fields); + dict_index_copy_types(tuple, index, old_n_fields); - rec_copy_prefix_to_dtuple(tuple, cursor->old_rec, index, - cursor->old_n_core_fields, - cursor->old_n_fields, heap); + rec_copy_prefix_to_dtuple(tuple, old_rec, index, + old_n_core_fields, + old_n_fields, heap); ut_ad(dtuple_check_typed(tuple)); /* Save the old search mode of the cursor */ - old_mode = cursor->search_mode; + old_mode = search_mode; - switch (cursor->rel_pos) { + switch (rel_pos) { case BTR_PCUR_ON: mode = PAGE_CUR_LE; break; @@ -420,41 +423,45 @@ btr_pcur_restore_position_func( mode = PAGE_CUR_UNSUPP; } - btr_pcur_open_with_no_init_func(index, tuple, mode, latch_mode, - cursor, + btr_pcur_open_with_no_init_func(index, tuple, mode, restore_latch_mode, + this, #ifdef BTR_CUR_HASH_ADAPT NULL, #endif /* BTR_CUR_HASH_ADAPT */ file, line, mtr); /* Restore the old search mode */ - cursor->search_mode = old_mode; + search_mode = old_mode; - ut_ad(cursor->rel_pos == BTR_PCUR_ON - || cursor->rel_pos == BTR_PCUR_BEFORE - || cursor->rel_pos == BTR_PCUR_AFTER); + ut_ad(rel_pos == BTR_PCUR_ON + || rel_pos == BTR_PCUR_BEFORE + || rel_pos == BTR_PCUR_AFTER); rec_offs offsets[REC_OFFS_NORMAL_SIZE]; rec_offs_init(offsets); - if (cursor->rel_pos == BTR_PCUR_ON - && btr_pcur_is_on_user_rec(cursor) - && !cmp_dtuple_rec(tuple, btr_pcur_get_rec(cursor), - rec_get_offsets(btr_pcur_get_rec(cursor), - index, offsets, - index->n_core_fields, - ULINT_UNDEFINED, &heap))) { + restore_status ret_val= NOT_SAME; + if (rel_pos == BTR_PCUR_ON && btr_pcur_is_on_user_rec(this)) { + ulint n_matched_fields= 0; + if (!cmp_dtuple_rec_with_match( + tuple, btr_pcur_get_rec(this), + rec_get_offsets(btr_pcur_get_rec(this), index, offsets, + index->n_core_fields, ULINT_UNDEFINED, &heap), + &n_matched_fields)) { - /* We have to store the NEW value for the modify clock, - since the cursor can now be on a different page! - But we can retain the value of old_rec */ + /* We have to store the NEW value for the modify clock, + since the cursor can now be on a different page! + But we can retain the value of old_rec */ - cursor->block_when_stored.store(btr_pcur_get_block(cursor)); - cursor->modify_clock = buf_block_get_modify_clock( - cursor->block_when_stored.block()); - cursor->old_stored = true; + block_when_stored.store(btr_pcur_get_block(this)); + modify_clock= buf_block_get_modify_clock( + block_when_stored.block()); + old_stored= true; - mem_heap_free(heap); + mem_heap_free(heap); - return(TRUE); + return SAME_ALL; + } + if (n_matched_fields >= index->n_uniq) + ret_val= SAME_UNIQ; } mem_heap_free(heap); @@ -463,9 +470,9 @@ btr_pcur_restore_position_func( to the cursor because it can now be on a different page, the record under it may have been removed, etc. */ - btr_pcur_store_position(cursor, mtr); + btr_pcur_store_position(this, mtr); - return(FALSE); + return ret_val; } /*********************************************************//** diff --git a/storage/innobase/fts/fts0fts.cc b/storage/innobase/fts/fts0fts.cc index 2a8c69d77fc..32982256302 100644 --- a/storage/innobase/fts/fts0fts.cc +++ b/storage/innobase/fts/fts0fts.cc @@ -3525,7 +3525,6 @@ fts_add_doc_by_id( get_doc, clust_index, doc_pcur, offsets, &doc); if (doc.found) { - ibool success MY_ATTRIBUTE((unused)); btr_pcur_store_position(doc_pcur, &mtr); mtr_commit(&mtr); @@ -3579,12 +3578,10 @@ fts_add_doc_by_id( mtr_start(&mtr); if (i < num_idx - 1) { - - success = btr_pcur_restore_position( - BTR_SEARCH_LEAF, doc_pcur, - &mtr); - - ut_ad(success); + ut_d(btr_pcur_t::restore_status status=) + btr_pcur_restore_position( + BTR_SEARCH_LEAF, doc_pcur, &mtr); + ut_ad(status == btr_pcur_t::SAME_ALL); } } diff --git a/storage/innobase/ibuf/ibuf0ibuf.cc b/storage/innobase/ibuf/ibuf0ibuf.cc index 4bebb7d1257..60496f20230 100644 --- a/storage/innobase/ibuf/ibuf0ibuf.cc +++ b/storage/innobase/ibuf/ibuf0ibuf.cc @@ -4175,7 +4175,8 @@ ibuf_restore_pos( ut_ad(mode == BTR_MODIFY_LEAF || BTR_LATCH_MODE_WITHOUT_INTENTION(mode) == BTR_MODIFY_TREE); - if (btr_pcur_restore_position(mode, pcur, mtr)) { + if (btr_pcur_restore_position(mode, pcur, mtr) == + btr_pcur_t::SAME_ALL) { return(TRUE); } diff --git a/storage/innobase/include/btr0pcur.h b/storage/innobase/include/btr0pcur.h index d8f9a0961e8..bbb9831ae93 100644 --- a/storage/innobase/include/btr0pcur.h +++ b/storage/innobase/include/btr0pcur.h @@ -253,31 +253,9 @@ btr_pcur_store_position( /*====================*/ btr_pcur_t* cursor, /*!< in: persistent cursor */ mtr_t* mtr); /*!< in: mtr */ -/**************************************************************//** -Restores the stored position of a persistent cursor bufferfixing the page and -obtaining the specified latches. If the cursor position was saved when the -(1) cursor was positioned on a user record: this function restores the position -to the last record LESS OR EQUAL to the stored record; -(2) cursor was positioned on a page infimum record: restores the position to -the last record LESS than the user record which was the successor of the page -infimum; -(3) cursor was positioned on the page supremum: restores to the first record -GREATER than the user record which was the predecessor of the supremum. -(4) cursor was positioned before the first or after the last in an empty tree: -restores to before first or after the last in the tree. -@return TRUE if the cursor position was stored when it was on a user -record and it can be restored on a user record whose ordering fields -are identical to the ones of the original user record */ -ibool -btr_pcur_restore_position_func( -/*===========================*/ - ulint latch_mode, /*!< in: BTR_SEARCH_LEAF, ... */ - btr_pcur_t* cursor, /*!< in: detached persistent cursor */ - const char* file, /*!< in: file name */ - unsigned line, /*!< in: line where called */ - mtr_t* mtr); /*!< in: mtr */ + #define btr_pcur_restore_position(l,cur,mtr) \ - btr_pcur_restore_position_func(l,cur,__FILE__,__LINE__,mtr) + (cur)->restore_position(l,__FILE__,__LINE__,mtr) /*********************************************************//** Gets the rel_pos field for a cursor whose position has been stored. @return BTR_PCUR_ON, ... */ @@ -482,6 +460,18 @@ enum pcur_pos_t { selects, updates, and deletes. */ struct btr_pcur_t{ + /** Return value of restore_position() */ + enum restore_status { + /** cursor position on user rec and points on the record with + the same field values as in the stored record */ + SAME_ALL, + /** cursor position is on user rec and points on the record with + the same unique field values as in the stored record */ + SAME_UNIQ, + /** cursor position is not on user rec or points on the record + with not the same uniq field values as in the stored record */ + NOT_SAME + }; /** a B-tree cursor */ btr_cur_t btr_cur; /** see TODO note below! @@ -538,6 +528,31 @@ struct btr_pcur_t{ /** Return the index of this persistent cursor */ dict_index_t* index() const { return(btr_cur.index); } + /** Restores the stored position of a persistent cursor bufferfixing + the page and obtaining the specified latches. If the cursor position + was saved when the + (1) cursor was positioned on a user record: this function restores the + position to the last record LESS OR EQUAL to the stored record; + (2) cursor was positioned on a page infimum record: restores the + position to the last record LESS than the user record which was the + successor of the page infimum; + (3) cursor was positioned on the page supremum: restores to the first + record GREATER than the user record which was the predecessor of the + supremum. + (4) cursor was positioned before the first or after the last in an + empty tree: restores to before first or after the last in the tree. + @param latch_mode BTR_SEARCH_LEAF, ... + @param file file name + @param line line where called + @param mtr mtr + @return btr_pcur_t::SAME_ALL cursor position on user rec and points on + the record with the same field values as in the stored record, + btr_pcur_t::SAME_UNIQ cursor position is on user rec and points on the + record with the same unique field values as in the stored record, + btr_pcur_t::NOT_SAME cursor position is not on user rec or points on + the record with not the samebuniq field values as in the stored */ + restore_status restore_position(ulint latch_mode, const char *file, + unsigned line, mtr_t *mtr); }; #include "btr0pcur.inl" diff --git a/storage/innobase/row/row0purge.cc b/storage/innobase/row/row0purge.cc index e57a63bc8e2..e46d515e8c2 100644 --- a/storage/innobase/row/row0purge.cc +++ b/storage/innobase/row/row0purge.cc @@ -73,7 +73,9 @@ row_purge_reposition_pcur( if (node->found_clust) { ut_ad(node->validate_pcur()); - node->found_clust = btr_pcur_restore_position(mode, &node->pcur, mtr); + node->found_clust = + btr_pcur_restore_position(mode, &node->pcur, mtr) == + btr_pcur_t::SAME_ALL; } else { node->found_clust = row_search_on_row_ref( @@ -252,7 +254,7 @@ static bool row_purge_restore_vsec_cur( return btr_pcur_restore_position( is_tree ? BTR_PURGE_TREE : BTR_PURGE_LEAF, - sec_pcur, sec_mtr); + sec_pcur, sec_mtr) == btr_pcur_t::SAME_ALL; } /** Determines if it is possible to remove a secondary index entry. diff --git a/storage/innobase/row/row0sel.cc b/storage/innobase/row/row0sel.cc index ed5b48b59e0..c718a53db61 100644 --- a/storage/innobase/row/row0sel.cc +++ b/storage/innobase/row/row0sel.cc @@ -1431,8 +1431,9 @@ row_sel_restore_pcur_pos( relative_position = btr_pcur_get_rel_pos(&(plan->pcur)); - equal_position = btr_pcur_restore_position(BTR_SEARCH_LEAF, - &(plan->pcur), mtr); + equal_position = + btr_pcur_restore_position(BTR_SEARCH_LEAF, &plan->pcur, mtr) == + btr_pcur_t::SAME_ALL; /* If the cursor is traveling upwards, and relative_position is @@ -3619,36 +3620,29 @@ err_exit: return(err); } -/********************************************************************//** -Restores cursor position after it has been stored. We have to take into +/** Restores cursor position after it has been stored. We have to take into account that the record cursor was positioned on may have been deleted. Then we may have to move the cursor one step up or down. +@param[out] same_user_rec true if we were able to restore the cursor on a user +record with the same ordering prefix in in the B-tree index +@param[in] latch_mode latch mode wished in restoration +@param[in] pcur cursor whose position has been stored +@param[in] moves_up true if the cursor moves up in the index +@param[in] mtr mtr; CAUTION: may commit mtr temporarily! +@param[in] select_lock_type select lock type: LOCK_NONE, LOCK_S, or LOCK_X @return true if we may need to process the record the cursor is now positioned on (i.e. we should not go to the next record yet) */ -static -bool -sel_restore_position_for_mysql( -/*===========================*/ - ibool* same_user_rec, /*!< out: TRUE if we were able to restore - the cursor on a user record with the - same ordering prefix in in the - B-tree index */ - ulint latch_mode, /*!< in: latch mode wished in - restoration */ - btr_pcur_t* pcur, /*!< in: cursor whose position - has been stored */ - ibool moves_up, /*!< in: TRUE if the cursor moves up - in the index */ - mtr_t* mtr) /*!< in: mtr; CAUTION: may commit - mtr temporarily! */ +static bool sel_restore_position_for_mysql(bool *same_user_rec, + ulint latch_mode, btr_pcur_t *pcur, + bool moves_up, mtr_t *mtr, + ulint select_lock_type) { - ibool success; + btr_pcur_t::restore_status status = btr_pcur_restore_position( + latch_mode, pcur, mtr); - success = btr_pcur_restore_position(latch_mode, pcur, mtr); + *same_user_rec = status == btr_pcur_t::SAME_ALL; - *same_user_rec = success; - - ut_ad(!success || pcur->rel_pos == BTR_PCUR_ON); + ut_ad(!*same_user_rec || pcur->rel_pos == BTR_PCUR_ON); #ifdef UNIV_DEBUG if (pcur->pos_state == BTR_PCUR_IS_POSITIONED_OPTIMISTIC) { ut_ad(pcur->rel_pos == BTR_PCUR_BEFORE @@ -3664,7 +3658,10 @@ sel_restore_position_for_mysql( switch (pcur->rel_pos) { case BTR_PCUR_ON: - if (!success && moves_up) { + if (!*same_user_rec && moves_up) { + if (status == btr_pcur_t::SAME_UNIQ + && select_lock_type != LOCK_NONE) + return true; next: if (btr_pcur_move_to_next(pcur, mtr) && rec_is_metadata(btr_pcur_get_rec(pcur), @@ -3674,7 +3671,7 @@ next: return true; } - return(!success); + return(!*same_user_rec); case BTR_PCUR_AFTER_LAST_IN_TREE: case BTR_PCUR_BEFORE_FIRST_IN_TREE: return true; @@ -4311,7 +4308,7 @@ row_search_mvcc( dberr_t err = DB_SUCCESS; ibool unique_search = FALSE; ibool mtr_has_extra_clust_latch = FALSE; - ibool moves_up = FALSE; + bool moves_up = false; ibool set_also_gap_locks = TRUE; /* if the query is a plain locking SELECT, and the isolation level is <= TRX_ISO_READ_COMMITTED, then this is set to FALSE */ @@ -4320,7 +4317,7 @@ row_search_mvcc( read (fetch the newest committed version), then this is set to TRUE */ ulint next_offs; - ibool same_user_rec; + bool same_user_rec; mtr_t mtr; mem_heap_t* heap = NULL; rec_offs offsets_[REC_OFFS_NORMAL_SIZE]; @@ -4624,10 +4621,10 @@ row_search_mvcc( if (UNIV_UNLIKELY(direction == 0)) { if (mode == PAGE_CUR_GE || mode == PAGE_CUR_G || mode >= PAGE_CUR_CONTAIN) { - moves_up = TRUE; + moves_up = true; } } else if (direction == ROW_SEL_NEXT) { - moves_up = TRUE; + moves_up = true; } thr = que_fork_get_first_thr(prebuilt->sel_graph); @@ -4677,7 +4674,7 @@ wait_table_again: bool need_to_process = sel_restore_position_for_mysql( &same_user_rec, BTR_SEARCH_LEAF, - pcur, moves_up, &mtr); + pcur, moves_up, &mtr, prebuilt->select_lock_type); if (UNIV_UNLIKELY(need_to_process)) { if (UNIV_UNLIKELY(prebuilt->row_read_type @@ -4899,7 +4896,7 @@ rec_loop: if (UNIV_UNLIKELY(next_offs >= srv_page_size - PAGE_DIR)) { wrong_offs: - if (srv_force_recovery == 0 || moves_up == FALSE) { + if (srv_force_recovery == 0 || moves_up == false) { ib::error() << "Rec address " << static_cast(rec) << ", buf block fix count " @@ -5695,7 +5692,9 @@ next_rec: if (sel_restore_position_for_mysql(&same_user_rec, BTR_SEARCH_LEAF, - pcur, moves_up, &mtr)) { + pcur, moves_up, &mtr, + prebuilt->select_lock_type) + ) { goto rec_loop; } } @@ -5786,7 +5785,7 @@ lock_table_wait: if (!dict_index_is_spatial(index)) { sel_restore_position_for_mysql( &same_user_rec, BTR_SEARCH_LEAF, pcur, - moves_up, &mtr); + moves_up, &mtr, prebuilt->select_lock_type); } if ((srv_locks_unsafe_for_binlog diff --git a/storage/innobase/row/row0uins.cc b/storage/innobase/row/row0uins.cc index 304ec71a63a..bf5c4e9b603 100644 --- a/storage/innobase/row/row0uins.cc +++ b/storage/innobase/row/row0uins.cc @@ -66,7 +66,6 @@ row_undo_ins_remove_clust_rec( undo_node_t* node) /*!< in: undo node */ { btr_cur_t* btr_cur; - ibool success; dberr_t err; ulint n_tries = 0; mtr_t mtr; @@ -99,11 +98,10 @@ row_undo_ins_remove_clust_rec( mtr_s_lock_index(index, &mtr); } - success = btr_pcur_restore_position( + ut_a(btr_pcur_restore_position( online ? BTR_MODIFY_LEAF | BTR_ALREADY_S_LATCHED - : BTR_MODIFY_LEAF, &node->pcur, &mtr); - ut_a(success); + : BTR_MODIFY_LEAF, &node->pcur, &mtr) == btr_pcur_t::SAME_ALL); btr_cur = btr_pcur_get_btr_cur(&node->pcur); @@ -137,9 +135,9 @@ row_undo_ins_remove_clust_rec( mtr.start(); - success = btr_pcur_restore_position( - BTR_MODIFY_LEAF, &node->pcur, &mtr); - ut_a(success); + ut_a(btr_pcur_restore_position( + BTR_MODIFY_LEAF, &node->pcur, &mtr) + == btr_pcur_t::SAME_ALL); break; case DICT_COLUMNS_ID: /* This is rolling back an INSERT into SYS_COLUMNS. @@ -204,11 +202,8 @@ retry: } else { index->set_modified(mtr); } - - success = btr_pcur_restore_position( - BTR_MODIFY_TREE | BTR_LATCH_FOR_DELETE, - &node->pcur, &mtr); - ut_a(success); + ut_a(btr_pcur_restore_position(BTR_MODIFY_TREE | BTR_LATCH_FOR_DELETE, + &node->pcur, &mtr) == btr_pcur_t::SAME_ALL); btr_cur_pessimistic_delete(&err, FALSE, btr_cur, 0, true, &mtr); diff --git a/storage/innobase/row/row0umod.cc b/storage/innobase/row/row0umod.cc index 88e87e2f9bc..cf32ea914c0 100644 --- a/storage/innobase/row/row0umod.cc +++ b/storage/innobase/row/row0umod.cc @@ -95,19 +95,14 @@ row_undo_mod_clust_low( btr_pcur_t* pcur; btr_cur_t* btr_cur; dberr_t err; -#ifdef UNIV_DEBUG - ibool success; -#endif /* UNIV_DEBUG */ pcur = &node->pcur; btr_cur = btr_pcur_get_btr_cur(pcur); -#ifdef UNIV_DEBUG - success = -#endif /* UNIV_DEBUG */ + ut_d(btr_pcur_t::restore_status pcur_restore_result =) btr_pcur_restore_position(mode, pcur, mtr); - ut_ad(success); + ut_ad(pcur_restore_result == btr_pcur_t::SAME_ALL); ut_ad(rec_get_trx_id(btr_cur_get_rec(btr_cur), btr_cur_get_index(btr_cur)) == thr_get_trx(thr)->id @@ -338,7 +333,8 @@ row_undo_mod_clust( ut_ad(node->new_trx_id); mtr.start(); - if (!btr_pcur_restore_position(BTR_MODIFY_LEAF, pcur, &mtr)) { + if (btr_pcur_restore_position(BTR_MODIFY_LEAF, pcur, &mtr) != + btr_pcur_t::SAME_ALL) { goto mtr_commit_exit; } @@ -360,9 +356,9 @@ row_undo_mod_clust( btr_pcur_commit_specify_mtr(pcur, &mtr); mtr.start(); - if (!btr_pcur_restore_position( + if (btr_pcur_restore_position( BTR_MODIFY_TREE | BTR_LATCH_FOR_DELETE, - pcur, &mtr)) { + pcur, &mtr) != btr_pcur_t::SAME_ALL) { goto mtr_commit_exit; } @@ -394,7 +390,8 @@ row_undo_mod_clust( longer accessible by any active read view. */ mtr.start(); - if (!btr_pcur_restore_position(BTR_MODIFY_LEAF, pcur, &mtr)) { + if (btr_pcur_restore_position(BTR_MODIFY_LEAF, pcur, &mtr) + != btr_pcur_t::SAME_ALL) { goto mtr_commit_exit; } rec_t* rec = btr_pcur_get_rec(pcur); @@ -467,7 +464,6 @@ row_undo_mod_del_mark_or_remove_sec_low( { btr_pcur_t pcur; btr_cur_t* btr_cur; - ibool success; dberr_t err = DB_SUCCESS; mtr_t mtr; mtr_t mtr_vers; @@ -539,9 +535,8 @@ row_undo_mod_del_mark_or_remove_sec_low( mtr_vers.start(); - success = btr_pcur_restore_position(BTR_SEARCH_LEAF, &(node->pcur), - &mtr_vers); - ut_a(success); + ut_a(btr_pcur_restore_position(BTR_SEARCH_LEAF, &node->pcur, &mtr_vers) + == btr_pcur_t::SAME_ALL); /* For temporary table, we can skip to check older version of clustered index entry, because there is no MVCC or purge. */ diff --git a/storage/innobase/row/row0upd.cc b/storage/innobase/row/row0upd.cc index d187f18b05c..cadfc181916 100644 --- a/storage/innobase/row/row0upd.cc +++ b/storage/innobase/row/row0upd.cc @@ -2925,7 +2925,8 @@ row_upd_clust_rec( the same transaction do not modify the record in the meantime. Therefore we can assert that the restoration of the cursor succeeds. */ - ut_a(btr_pcur_restore_position(BTR_MODIFY_TREE, pcur, mtr)); + ut_a(btr_pcur_restore_position(BTR_MODIFY_TREE, pcur, mtr) == + btr_pcur_t::SAME_ALL); ut_ad(!rec_get_deleted_flag(btr_pcur_get_rec(pcur), dict_table_is_comp(index->table))); @@ -3128,7 +3129,8 @@ row_upd_clust_step( mode = BTR_MODIFY_LEAF; } - if (!btr_pcur_restore_position(mode, pcur, &mtr)) { + if (btr_pcur_restore_position(mode, pcur, &mtr) != + btr_pcur_t::SAME_ALL) { err = DB_RECORD_NOT_FOUND; goto exit_func; } @@ -3150,7 +3152,8 @@ row_upd_clust_step( mtr.start(); index->set_modified(mtr); - if (!btr_pcur_restore_position(BTR_MODIFY_LEAF, pcur, &mtr)) { + if (btr_pcur_restore_position(BTR_MODIFY_LEAF, pcur, &mtr) != + btr_pcur_t::SAME_ALL) { err = DB_ERROR; goto exit_func; }