diff --git a/mysql-test/suite/innodb/r/cursor_bug.result b/mysql-test/suite/innodb/r/cursor_bug.result new file mode 100644 index 00000000000..dbf69fc0cd5 --- /dev/null +++ b/mysql-test/suite/innodb/r/cursor_bug.result @@ -0,0 +1,52 @@ +SET transaction_isolation = "READ-UNCOMMITTED"; +SET GLOBAL innodb_limit_optimistic_insert_debug = 4; +SET GLOBAL debug = "+d,innobase_stop_merge"; +CREATE TABLE t1 (c INTEGER NOT NULL, PRIMARY KEY (c)) ENGINE=INNODB; +# Root Page: (1,3) +# Leaf Page: (1,2), (3,6,8) +INSERT INTO t1 VALUES (1), (2), (3), (6), (8); +# 1. Scan the table in reverse order +SELECT * FROM t1 ORDER BY c DESC; +c +8 +6 +3 +2 +1 +# 2. Scan the table while inserting a row, optimistic restore +# Root Page: (1,3) +# Leaf Page: (1,2), (6,8) +DELETE FROM t1 WHERE c = 3; +SET debug_sync = "move_backward_from_page_sync SIGNAL continue_insert WAIT_FOR continue_scan"; +SELECT * FROM t1 ORDER BY c DESC; +SET debug_sync = "now WAIT_FOR continue_insert"; +# Root Page: (1,3) +# Leaf Page: (1,2), (5,6,8) +INSERT INTO t1 VALUES (5); +SET debug_sync = "now SIGNAL continue_scan"; +c +8 +6 +5 +2 +1 +# 3. Scan the table while inserting a row, pessimistic restore +SET debug_sync = "move_backward_from_page_sync SIGNAL continue_insert WAIT_FOR continue_scan"; +SELECT * FROM t1 ORDER BY c DESC; +SET debug_sync = "now WAIT_FOR continue_insert"; +# Root Page: (1,3,6) +# Leaf Page: (1,2), (4,5), (6,7,8) // Page Split causes modify_clock+1, need to restore pessimisticly +INSERT INTO t1 VALUES (4); +INSERT INTO t1 VALUES (7); +SET debug_sync = "now SIGNAL continue_scan"; +c +8 +6 +5 +4 +2 +1 +# 4. Cleanup +SET GLOBAL innodb_limit_optimistic_insert_debug = 0; +SET GLOBAL debug = "-d,innobase_stop_merge"; +DROP TABLE t1; diff --git a/mysql-test/suite/innodb/t/cursor_bug.test b/mysql-test/suite/innodb/t/cursor_bug.test new file mode 100644 index 00000000000..4b10d2bb6c2 --- /dev/null +++ b/mysql-test/suite/innodb/t/cursor_bug.test @@ -0,0 +1,59 @@ +--source include/have_debug.inc +--source include/have_debug_sync.inc + +--connect(con1,localhost,root) +--connect(con2,localhost,root) + +--connection con1 +SET transaction_isolation = "READ-UNCOMMITTED"; +SET GLOBAL innodb_limit_optimistic_insert_debug = 4; +SET GLOBAL debug = "+d,innobase_stop_merge"; +--sleep 5 + +CREATE TABLE t1 (c INTEGER NOT NULL, PRIMARY KEY (c)) ENGINE=INNODB; + +--echo # Root Page: (1,3) +--echo # Leaf Page: (1,2), (3,6,8) +INSERT INTO t1 VALUES (1), (2), (3), (6), (8); + +--echo # 1. Scan the table in reverse order +SELECT * FROM t1 ORDER BY c DESC; + +--echo # 2. Scan the table while inserting a row, optimistic restore +--echo # Root Page: (1,3) +--echo # Leaf Page: (1,2), (6,8) +DELETE FROM t1 WHERE c = 3; +--source include/wait_innodb_all_purged.inc + +SET debug_sync = "move_backward_from_page_sync SIGNAL continue_insert WAIT_FOR continue_scan"; +send SELECT * FROM t1 ORDER BY c DESC; + +--connection con2 +SET debug_sync = "now WAIT_FOR continue_insert"; +--echo # Root Page: (1,3) +--echo # Leaf Page: (1,2), (5,6,8) +INSERT INTO t1 VALUES (5); +SET debug_sync = "now SIGNAL continue_scan"; + +--connection con1 +reap; + +--echo # 3. Scan the table while inserting a row, pessimistic restore +SET debug_sync = "move_backward_from_page_sync SIGNAL continue_insert WAIT_FOR continue_scan"; +send SELECT * FROM t1 ORDER BY c DESC; + +--connection con2 +SET debug_sync = "now WAIT_FOR continue_insert"; +--echo # Root Page: (1,3,6) +--echo # Leaf Page: (1,2), (4,5), (6,7,8) // Page Split causes modify_clock+1, need to restore pessimisticly +INSERT INTO t1 VALUES (4); +INSERT INTO t1 VALUES (7); +SET debug_sync = "now SIGNAL continue_scan"; + +--connection con1 +reap; + +--echo # 4. Cleanup +SET GLOBAL innodb_limit_optimistic_insert_debug = 0; +SET GLOBAL debug = "-d,innobase_stop_merge"; +DROP TABLE t1; \ No newline at end of file diff --git a/storage/innobase/btr/btr0pcur.cc b/storage/innobase/btr/btr0pcur.cc index 8745ead0124..fcec3975b8e 100644 --- a/storage/innobase/btr/btr0pcur.cc +++ b/storage/innobase/btr/btr0pcur.cc @@ -35,6 +35,8 @@ this program; if not, write to the Free Software Foundation, Inc., #include +#include "current_thd.h" +#include "debug_sync.h" #include "rem0cmp.h" #include "trx0trx.h" #include "ut0byte.h" @@ -91,12 +93,14 @@ void btr_pcur_t::store_position(mtr_t *mtr) { } if (page_rec_is_supremum_low(offs)) { - rec = page_rec_get_prev(rec); + move_to_prev_on_page(); + rec = page_cur_get_rec(page_cursor); m_rel_pos = BTR_PCUR_AFTER; } else if (page_rec_is_infimum_low(offs)) { - rec = page_rec_get_next(rec); + move_to_next_on_page(); + rec = page_cur_get_rec(page_cursor); m_rel_pos = BTR_PCUR_BEFORE; } else { @@ -220,12 +224,12 @@ bool btr_pcur_t::restore_position(ulint latch_mode, mtr_t *mtr, return (true); } - /* This is the same record as stored, - may need to be adjusted for BTR_PCUR_BEFORE/AFTER, - depending on search mode and direction. */ - if (is_on_user_rec()) { - m_pos_state = BTR_PCUR_IS_POSITIONED_OPTIMISTIC; + if (m_rel_pos == BTR_PCUR_AFTER) { + move_to_next_on_page(); + } else if (m_rel_pos == BTR_PCUR_BEFORE) { + move_to_prev_on_page(); } + return (false); } } @@ -287,8 +291,9 @@ bool btr_pcur_t::restore_position(ulint latch_mode, mtr_t *mtr, /* We have to store new position information, modify_clock etc., to the cursor because it can now be on a different page, the record under it may have been removed, etc. */ - + auto old_rel_pos = m_rel_pos; store_position(mtr); + m_rel_pos = old_rel_pos; return (false); } @@ -375,6 +380,8 @@ void btr_pcur_t::move_backward_from_page(mtr_t *mtr) { mtr_commit(mtr); + DEBUG_SYNC(current_thd, "move_backward_from_page_sync"); + mtr_start(mtr); restore_position(latch_mode2, mtr, UT_LOCATION_HERE); diff --git a/storage/innobase/include/btr0cur.ic b/storage/innobase/include/btr0cur.ic index 360d3d02b29..4e871eef7fb 100644 --- a/storage/innobase/include/btr0cur.ic +++ b/storage/innobase/include/btr0cur.ic @@ -145,6 +145,9 @@ static inline bool btr_cur_can_delete_without_compress( page = btr_cur_get_page(cursor); + DBUG_EXECUTE_IF("innobase_stop_merge", + if (strstr(cursor->index->table_name, "t1")) return true;); + if ((page_get_data_size(page) - rec_size < BTR_CUR_PAGE_COMPRESS_LIMIT(cursor->index)) || ((btr_page_get_next(page, mtr) == FIL_NULL) && diff --git a/storage/innobase/include/btr0pcur.h b/storage/innobase/include/btr0pcur.h index b8690d9f418..a80350011de 100644 --- a/storage/innobase/include/btr0pcur.h +++ b/storage/innobase/include/btr0pcur.h @@ -76,12 +76,6 @@ enum pcur_pos_t { all current code should be ok. */ BTR_PCUR_WAS_POSITIONED, - /** The persistent cursor is positioned by optimistic get to the same - record as it was positioned at. Not used for rel_pos == BTR_PCUR_ON. - It may need adjustment depending on previous/current search direction - and rel_pos. */ - BTR_PCUR_IS_POSITIONED_OPTIMISTIC, - /** The persistent cursor is positioned by index search. Or optimistic get for rel_pos == BTR_PCUR_ON. */ BTR_PCUR_IS_POSITIONED diff --git a/storage/innobase/row/row0pread.cc b/storage/innobase/row/row0pread.cc index b4755492df5..11983054901 100644 --- a/storage/innobase/row/row0pread.cc +++ b/storage/innobase/row/row0pread.cc @@ -265,13 +265,7 @@ class PCursor { auto equal = m_pcur->restore_position(MODE, m_mtr, UT_LOCATION_HERE); #ifdef UNIV_DEBUG - if (m_pcur->m_pos_state == BTR_PCUR_IS_POSITIONED_OPTIMISTIC) { - ut_ad(m_pcur->m_rel_pos == BTR_PCUR_BEFORE || - m_pcur->m_rel_pos == BTR_PCUR_AFTER); - } else { - ut_ad(m_pcur->m_pos_state == BTR_PCUR_IS_POSITIONED); - ut_ad((m_pcur->m_rel_pos == BTR_PCUR_ON) == m_pcur->is_on_user_rec()); - } + ut_ad(m_pcur->m_pos_state == BTR_PCUR_IS_POSITIONED); #endif /* UNIV_DEBUG */ switch (relative) { @@ -291,27 +285,10 @@ class PCursor { break; case BTR_PCUR_BEFORE: - /* For non-optimistic restoration: - The position is now set to the record before pcur->old_rec. - - For optimistic restoration: - The position also needs to take the previous search_mode into - consideration. */ - switch (m_pcur->m_pos_state) { - case BTR_PCUR_IS_POSITIONED_OPTIMISTIC: - m_pcur->m_pos_state = BTR_PCUR_IS_POSITIONED; - /* The cursor always moves "up" ie. in ascending order. */ - break; - - case BTR_PCUR_IS_POSITIONED: - if (m_pcur->is_on_user_rec()) { - m_pcur->move_to_next(m_mtr); - } - break; - - case BTR_PCUR_NOT_POSITIONED: - case BTR_PCUR_WAS_POSITIONED: - ut_error; + ut_a(m_pcur->m_pos_state == BTR_PCUR_IS_POSITIONED); + /* The cursor always moves "up" ie. in ascending order. */ + if (m_pcur->is_on_user_rec()) { + m_pcur->move_to_next(m_mtr); } break; } @@ -406,10 +383,6 @@ void PCursor::restore_to_last_processed_user_record() noexcept { [[maybe_unused]] const auto same_row = m_pcur->restore_position(BTR_SEARCH_LEAF, m_mtr, UT_LOCATION_HERE); - /* Since m_rel_pos was BTR_PCUR_ON, the state can't be - BTR_PCUR_IS_POSITIONED_OPTIMISTIC */ - ut_a(BTR_PCUR_IS_POSITIONED_OPTIMISTIC != m_pcur->m_pos_state); - /* If the cursor was pointing to the user record then 'PAGE_CUR_LE' mode used by the btr_pcur_t::restore_position() restores the cursor to a record lower than the original one if it was removed, and in particular, if all @@ -437,15 +410,6 @@ void PCursor::restore_to_first_unprocessed() noexcept { m_mtr->start(); m_mtr->set_log_mode(MTR_LOG_NO_REDO); m_pcur->restore_position(BTR_SEARCH_LEAF, m_mtr, UT_LOCATION_HERE); - if (m_pcur->m_pos_state == BTR_PCUR_IS_POSITIONED_OPTIMISTIC) { - /* The BTR_PCUR_IS_POSITIONED_OPTIMISTIC means the implementation of - btr_pcur_t::restore_position() successfully re-acquired a block stored in - hint and it wasn't modified meanwhile, and in such case it does not advance - the cursor to the next position even though BTR_PCUR_AFTER was used, so we - have to do it ourselves. */ - ut_a(!m_pcur->is_after_last_on_page()); - m_pcur->move_to_next_on_page(); - } } dberr_t PCursor::move_to_user_rec() noexcept { diff --git a/storage/innobase/row/row0sel.cc b/storage/innobase/row/row0sel.cc index 0c4c7fc86c2..625ebdb3de7 100644 --- a/storage/innobase/row/row0sel.cc +++ b/storage/innobase/row/row0sel.cc @@ -3415,13 +3415,7 @@ static bool sel_restore_position_for_mysql( ut_ad(!success || pcur->m_rel_pos == BTR_PCUR_ON); #ifdef UNIV_DEBUG - if (pcur->m_pos_state == BTR_PCUR_IS_POSITIONED_OPTIMISTIC) { - ut_ad(pcur->m_rel_pos == BTR_PCUR_BEFORE || - pcur->m_rel_pos == BTR_PCUR_AFTER); - } else { - ut_ad(pcur->m_pos_state == BTR_PCUR_IS_POSITIONED); - ut_ad((pcur->m_rel_pos == BTR_PCUR_ON) == pcur->is_on_user_rec()); - } + ut_ad(pcur->m_pos_state == BTR_PCUR_IS_POSITIONED); #endif /* UNIV_DEBUG */ /* The position may need be adjusted for rel_pos and moves_up. */ @@ -3432,7 +3426,6 @@ static bool sel_restore_position_for_mysql( ut_o(return (true)); case BTR_PCUR_ON: if (!success && moves_up) { - next: pcur->move_to_next(mtr); return true; } @@ -3443,42 +3436,17 @@ static bool sel_restore_position_for_mysql( case BTR_PCUR_AFTER: /* positioned to record after pcur->old_rec. */ pcur->m_pos_state = BTR_PCUR_IS_POSITIONED; - prev: if (pcur->is_on_user_rec() && !moves_up) { pcur->move_to_prev(mtr); } return true; case BTR_PCUR_BEFORE: - /* For non optimistic restoration: - The position is now set to the record before pcur->old_rec. - - For optimistic restoration: - The position also needs to take the previous search_mode into - consideration. */ - - switch (pcur->m_pos_state) { - case BTR_PCUR_IS_POSITIONED_OPTIMISTIC: - pcur->m_pos_state = BTR_PCUR_IS_POSITIONED; - if (pcur->m_search_mode == PAGE_CUR_GE) { - /* Positioned during Greater or Equal search - with BTR_PCUR_BEFORE. Optimistic restore to - the same record. If scanning for lower then - we must move to previous record. - This can happen with: - HANDLER READ idx a = (const); - HANDLER READ idx PREV; */ - goto prev; - } - return true; - case BTR_PCUR_IS_POSITIONED: - if (moves_up && pcur->is_on_user_rec()) { - goto next; - } - return true; - case BTR_PCUR_WAS_POSITIONED: - case BTR_PCUR_NOT_POSITIONED: - break; + /* positioned to record before pcur->old_rec. */ + pcur->m_pos_state = BTR_PCUR_IS_POSITIONED; + if (moves_up && pcur->is_on_user_rec()) { + pcur->move_to_next(mtr); } + return true; } ut_d(ut_error); ut_o(return (true));