Bug#13807811 BTR_PCUR_RESTORE_POSITION() CAN SKIP A RECORD

This bug has been there at least since MySQL 4.0.9. (Before 4.0.9, the
code probably was even more severely broken.)

btr_pcur_restore_position(): When cursor restoration fails, before
invoking btr_pcur_store_position() move to the previous or next record
unless cursor->rel_pos==BTR_PCUR_ON or the record was not a user
record.

This bug can cause skipped records when btr_pcur_store_position() is
called on the last record of a page. A symptom would be record count
mismatch in CHECK TABLE, or failure to find a record to delete-mark or
update or purge. The following operations should be affected by the
bug:

* row_search_for_mysql(): SELECT, UPDATE, REPLACE, CHECK TABLE,
  (almost anything else than INSERT)

* foreign key CASCADE operations

* row_merge_read_clustered_index(): index creation (since MySQL 5.1
  InnoDB Plugin)

* multi-threaded purge (after MySQL 5.5): not sure, but it might fail
  to purge some records

Not all callers of btr_pcur_restore_position() should be affected.
Anything that asserts or checks that restoration succeeds is
unaffected. For example, cursor restoration on the change buffer tree
should always succeed, because access is being protected by additional
latches. Likewise, rollback, or any code accesses data dictionary
tables while holding dict_sys->mutex should be safe.

rb:967 approved by Jimmy Yang

storage/innobase/btr/btr0pcur.c

@@ -291,13 +291,20 @@ btr_pcur_restore_position(
 	/* Save the old search mode of the cursor */
 	old_mode = cursor->search_mode;
 
-	if (UNIV_LIKELY(cursor->rel_pos == BTR_PCUR_ON)) {
+	switch (cursor->rel_pos) {
+	case BTR_PCUR_ON:
 		mode = PAGE_CUR_LE;
-	} else if (cursor->rel_pos == BTR_PCUR_AFTER) {
+		break;
+	case BTR_PCUR_AFTER:
 		mode = PAGE_CUR_G;
-	} else {
-		ut_ad(cursor->rel_pos == BTR_PCUR_BEFORE);
+		break;
+	case BTR_PCUR_BEFORE:
 		mode = PAGE_CUR_L;
+		break;
+#ifdef UNIV_DEBUG
+	default:
+		ut_error;
+#endif /* UNIV_DEBUG */
 	}
 
 	btr_pcur_open_with_no_init(index, tuple, mode, latch_mode,
@@ -306,26 +313,45 @@ btr_pcur_restore_position(
 	/* Restore the old search mode */
 	cursor->search_mode = old_mode;
 
-	if (cursor->rel_pos == BTR_PCUR_ON
-	    && btr_pcur_is_on_user_rec(cursor, mtr)
-	    && 0 == cmp_dtuple_rec(tuple, btr_pcur_get_rec(cursor),
-				   rec_get_offsets(
-					   btr_pcur_get_rec(cursor), index,
-					   NULL, ULINT_UNDEFINED, &heap))) {
-
-		/* 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 = buf_block_align(
-			btr_pcur_get_page(cursor));
-		cursor->modify_clock = buf_block_get_modify_clock(
-			cursor->block_when_stored);
-		cursor->old_stored = BTR_PCUR_OLD_STORED;
+	if (btr_pcur_is_on_user_rec(cursor, mtr)) {
+		switch (cursor->rel_pos) {
+		case BTR_PCUR_ON:
+			if (!cmp_dtuple_rec(
+				    tuple, btr_pcur_get_rec(cursor),
+				    rec_get_offsets(btr_pcur_get_rec(cursor),
+						    index, NULL,
+						    ULINT_UNDEFINED, &heap))) {
+
+				/* 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 =
+					buf_block_align(
+						btr_pcur_get_page(cursor));
+				cursor->modify_clock =
+					buf_block_get_modify_clock(
+						cursor->block_when_stored);
+				cursor->old_stored = BTR_PCUR_OLD_STORED;
 
-		mem_heap_free(heap);
+				mem_heap_free(heap);
 
-		return(TRUE);
+				return(TRUE);
+			}
+
+			break;
+		case BTR_PCUR_BEFORE:
+			page_cur_move_to_next(btr_pcur_get_page_cur(cursor));
+			break;
+		case BTR_PCUR_AFTER:
+			page_cur_move_to_prev(btr_pcur_get_page_cur(cursor));
+			break;
+#ifdef UNIV_DEBUG
+		default:
+			ut_error;
+#endif /* UNIV_DEBUG */
+		}
 	}
 
 	mem_heap_free(heap);

storage/innodb_plugin/ChangeLog

+2012-03-08	The InnoDB Team
+
+	* btr/btr0pcur.c:
+	Fix Bug#13807811 BTR_PCUR_RESTORE_POSITION() CAN SKIP A RECORD
+
 2012-02-28	The InnoDB Team
 
 	* btr/btr0btr.c, dict/dict0dict.c, include/btr0btr.h,

storage/innodb_plugin/btr/btr0pcur.c

 /*****************************************************************************
 
-Copyright (c) 1996, 2011, Oracle and/or its affiliates. All Rights Reserved.
+Copyright (c) 1996, 2012, Oracle and/or its affiliates. All Rights Reserved.
 
 This program is free software; you can redistribute it and/or modify it under
 the terms of the GNU General Public License as published by the Free Software
@@ -121,6 +121,8 @@ btr_pcur_store_position(
 
 		ut_a(btr_page_get_next(page, mtr) == FIL_NULL);
 		ut_a(btr_page_get_prev(page, mtr) == FIL_NULL);
+		ut_ad(page_is_leaf(page));
+		ut_ad(page_get_page_no(page) == index->page);
 
 		cursor->old_stored = BTR_PCUR_OLD_STORED;
 
@@ -313,13 +315,20 @@ btr_pcur_restore_position_func(
 	/* Save the old search mode of the cursor */
 	old_mode = cursor->search_mode;
 
-	if (UNIV_LIKELY(cursor->rel_pos == BTR_PCUR_ON)) {
+	switch (cursor->rel_pos) {
+	case BTR_PCUR_ON:
 		mode = PAGE_CUR_LE;
-	} else if (cursor->rel_pos == BTR_PCUR_AFTER) {
+		break;
+	case BTR_PCUR_AFTER:
 		mode = PAGE_CUR_G;
-	} else {
-		ut_ad(cursor->rel_pos == BTR_PCUR_BEFORE);
+		break;
+	case BTR_PCUR_BEFORE:
 		mode = PAGE_CUR_L;
+		break;
+#ifdef UNIV_DEBUG
+	default:
+		ut_error;
+#endif /* UNIV_DEBUG */
 	}
 
 	btr_pcur_open_with_no_init_func(index, tuple, mode, latch_mode,
@@ -328,25 +337,44 @@ btr_pcur_restore_position_func(
 	/* Restore the old search mode */
 	cursor->search_mode = old_mode;
 
-	if (cursor->rel_pos == BTR_PCUR_ON
-	    && btr_pcur_is_on_user_rec(cursor)
-	    && 0 == cmp_dtuple_rec(tuple, btr_pcur_get_rec(cursor),
-				   rec_get_offsets(
-					   btr_pcur_get_rec(cursor), index,
-					   NULL, ULINT_UNDEFINED, &heap))) {
+	if (btr_pcur_is_on_user_rec(cursor)) {
+		switch (cursor->rel_pos) {
+		case BTR_PCUR_ON:
+			if (!cmp_dtuple_rec(
+				    tuple, btr_pcur_get_rec(cursor),
+				    rec_get_offsets(btr_pcur_get_rec(cursor),
+						    index, NULL,
+						    ULINT_UNDEFINED, &heap))) {
+
+				/* 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 =
+					btr_pcur_get_block(cursor);
+				cursor->modify_clock =
+					buf_block_get_modify_clock(
+						cursor->block_when_stored);
+				cursor->old_stored = BTR_PCUR_OLD_STORED;
 
-		/* 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 = btr_pcur_get_block(cursor);
-		cursor->modify_clock = buf_block_get_modify_clock(
-			cursor->block_when_stored);
-		cursor->old_stored = BTR_PCUR_OLD_STORED;
+				mem_heap_free(heap);
 
-		mem_heap_free(heap);
+				return(TRUE);
+			}
 
-		return(TRUE);
+			break;
+		case BTR_PCUR_BEFORE:
+			page_cur_move_to_next(btr_pcur_get_page_cur(cursor));
+			break;
+		case BTR_PCUR_AFTER:
+			page_cur_move_to_prev(btr_pcur_get_page_cur(cursor));
+			break;
+#ifdef UNIV_DEBUG
+		default:
+			ut_error;
+#endif /* UNIV_DEBUG */
+		}
 	}
 
 	mem_heap_free(heap);