Bug#36846567 Inplace ALTER TABLE might cause lost rows if concurrent purge [Post-push fix]

Bug#37318367 Inplace ALTER on table with spatial idx might cause lost rows if concurrent purge [Post-push fix]

Description:
------------
This is post push fix of the above Bugs. There is another method
PCursor::move_to_next_block() where the scan is paused and latches are
released. This method was missed to be addressed, as a result
ALTER TABLE rebuild might lose records if latches are released in
this case.

Background:
-----------
When doing a parallel scan, Parallel_reader::Ctx::traverse() calls
Parallel_reader::Ctx::traverse_recs() which does:

```
  /* Note: The page end callback (above) can save and restore the cursor.
  The restore can end up in the middle of a page. */
  if (pcursor->is_after_last_on_page() && !move_to_next_node(pcursor)) {
    break;
  }
```
Where Parallel_reader::Ctx::move_to_next_node() calls
PCursor::move_to_next_block which uses PCursor::savepoint() and
PCursor::restore_from_savepoint().

These functions are known to have problems which were addressed in the
recent bug fixes, but they missed to fix this particular code path.
The reason for that omission was that the PCursor::move_to_next_block()
is also used during SELECT COUNT(*) at internal level (above leaves) for
which the solution employed for ALTER will not work, because B-tree
navigation functions do not support the required comparison mode when
searching to non-leaf level.

Fix:
----
This patch carves out the part of PCursor::move_to_next_block() which is
handling the simple leaf-level case, for which it is well known what
should be done: we are at supremum, all previous records were fully
processed, and we should restore the cursor to a user record which is
larger than any processed record. That can be accomplished by using the
recently introduced methods save_previous_user_record_as_last_processed()
and restore_to_first_unprocessed(), with a small adjustment, that here we
require the cursor to be not only after all processed records, but also
on a user record, so in case it is restored to supremum, we should
additionally call move_to_user_rec().

This patch also makes it clear which methods are used, and where, by
removing unused methods, marking some of them as private, and adding
asserts that m_read_level!=0 to those related to SELECT COUNT() only.

Change-Id: I2994374c542e168e0884e947634593ade17495ec

Author: Jakub Łopuszański

mysql-test/suite/innodb/r/alter_table_rebuild_missing_record.result

@@ -1,5 +1,12 @@
-SET DEBUG_SYNC= 'RESET';
 connect  con1, localhost, root,,;
+CREATE PROCEDURE test.insert_records (IN record_count INT)
+BEGIN
+DECLARE counter INT DEFAULT 1;
+WHILE counter <= record_count DO
+INSERT INTO test.t1 VALUES (0);
+SET counter = counter + 1;
+END WHILE;
+END //
 ##############################################################
 # Bug#36846567 Inplace ALTER TABLE might cause lost rows if concurrent purge
 ##############################################################
@@ -10,7 +17,7 @@ INSERT INTO t1 VALUES ('aaaaa'), ('bbbbb'), ('bbbcc'), ('ccccc'), ('ddddd'), ('e
 SET GLOBAL INNODB_PURGE_STOP_NOW=ON;
 DELETE FROM t1 WHERE pk = 'bbbcc';
 connection con1;
-SET DEBUG='+d,ddl_buf_add_two';
+SET SESSION DEBUG='+d,ddl_buf_add_two';
 SET DEBUG_SYNC='ddl_bulk_inserter_latches_released SIGNAL latches_released WAIT_FOR go';
 # Send ALTER TABLE INPLACE which rebuilds table.
 ALTER TABLE t1 ENGINE=InnoDB, ALGORITHM=INPLACE;
@@ -20,6 +27,7 @@ SET GLOBAL innodb_purge_run_now=ON;
 SET DEBUG_SYNC='now SIGNAL go';
 connection con1;
 # Reap ALTER TABLE
+SET SESSION DEBUG='-d,ddl_buf_add_two';
 # Before the fix row 'ddddd' was missing from the table after ALTER.
 SELECT * FROM t1;
 pk
@@ -28,10 +36,8 @@ bbbbb
 ccccc
 ddddd
 eeeee
-SET DEBUG_SYNC= 'RESET';
 # Test cleanup
 connection default;
-SET DEBUG_SYNC= 'RESET';
 DROP TABLE t1;
 ##############################################################
 # Test#2 Delete the first record
@@ -43,7 +49,7 @@ INSERT INTO t1 VALUES ('aaaaa'), ('bbbbb'), ('bbbcc'), ('ccccc'), ('ddddd'), ('e
 SET GLOBAL INNODB_PURGE_STOP_NOW=ON;
 DELETE FROM t1 WHERE pk = 'aaaaa';
 connection con1;
-SET DEBUG='+d,ddl_buf_add_two';
+SET SESSION DEBUG='+d,ddl_buf_add_two';
 SET DEBUG_SYNC='ddl_bulk_inserter_latches_released SIGNAL latches_released WAIT_FOR go';
 # Send ALTER TABLE INPLACE which rebuilds table.
 ALTER TABLE t1 ENGINE=InnoDB, ALGORITHM=INPLACE;
@@ -53,6 +59,7 @@ SET GLOBAL innodb_purge_run_now=ON;
 SET DEBUG_SYNC='now SIGNAL go';
 connection con1;
 # Reap ALTER TABLE
+SET SESSION DEBUG='-d,ddl_buf_add_two';
 # Verify intended records are present
 SELECT * FROM t1;
 pk
@@ -61,10 +68,8 @@ bbbcc
 ccccc
 ddddd
 eeeee
-SET DEBUG_SYNC= 'RESET';
 # Test cleanup
 connection default;
-SET DEBUG_SYNC= 'RESET';
 DROP TABLE t1;
 ##############################################################
 # Test#3 Delete the second record
@@ -76,7 +81,7 @@ INSERT INTO t1 VALUES ('aaaaa'), ('bbbbb'), ('bbbcc'), ('ccccc'), ('ddddd'), ('e
 SET GLOBAL INNODB_PURGE_STOP_NOW=ON;
 DELETE FROM t1 WHERE pk = 'bbbbb';
 connection con1;
-SET DEBUG='+d,ddl_buf_add_two';
+SET SESSION DEBUG='+d,ddl_buf_add_two';
 SET DEBUG_SYNC='ddl_bulk_inserter_latches_released SIGNAL latches_released WAIT_FOR go';
 # Send ALTER TABLE INPLACE which rebuilds table.
 ALTER TABLE t1 ENGINE=InnoDB, ALGORITHM=INPLACE;
@@ -86,6 +91,7 @@ SET GLOBAL innodb_purge_run_now=ON;
 SET DEBUG_SYNC='now SIGNAL go';
 connection con1;
 # Reap ALTER TABLE
+SET SESSION DEBUG='-d,ddl_buf_add_two';
 # Verify intended records are present
 SELECT * FROM t1;
 pk
@@ -94,10 +100,8 @@ bbbcc
 ccccc
 ddddd
 eeeee
-SET DEBUG_SYNC= 'RESET';
 # Test cleanup
 connection default;
-SET DEBUG_SYNC= 'RESET';
 DROP TABLE t1;
 ##############################################################
 # Test#4 Delete the fourth record
@@ -109,7 +113,7 @@ INSERT INTO t1 VALUES ('aaaaa'), ('bbbbb'), ('bbbcc'), ('ccccc'), ('ddddd'), ('e
 SET GLOBAL INNODB_PURGE_STOP_NOW=ON;
 DELETE FROM t1 WHERE pk = 'ccccc';
 connection con1;
-SET DEBUG='+d,ddl_buf_add_two';
+SET SESSION DEBUG='+d,ddl_buf_add_two';
 SET DEBUG_SYNC='ddl_bulk_inserter_latches_released SIGNAL latches_released WAIT_FOR go';
 # Send ALTER TABLE INPLACE which rebuilds table.
 ALTER TABLE t1 ENGINE=InnoDB, ALGORITHM=INPLACE;
@@ -119,6 +123,7 @@ SET GLOBAL innodb_purge_run_now=ON;
 SET DEBUG_SYNC='now SIGNAL go';
 connection con1;
 # Reap ALTER TABLE
+SET SESSION DEBUG='-d,ddl_buf_add_two';
 # Verify intended records are present
 SELECT * FROM t1;
 pk
@@ -127,14 +132,43 @@ bbbbb
 bbbcc
 ddddd
 eeeee
-SET DEBUG_SYNC= 'RESET';
 # Test cleanup
 connection default;
-SET DEBUG_SYNC= 'RESET';
 DROP TABLE t1;
+############################################################
+# Test#5 Bug#37318367 Delete part of records from two pages
+# to lose a record - pause in PCursor::move_to_next_block
+############################################################
+SET GLOBAL innodb_limit_optimistic_insert_debug=3;
+connection default;
+CREATE TABLE t1 (id int AUTO_INCREMENT NOT NULL, PRIMARY KEY(id)) ENGINE=InnoDB;
+CALL insert_records(8);
+SET GLOBAL innodb_purge_stop_now = ON;
+DELETE FROM t1 WHERE id > 2 AND id < 7;
+connection con1;
+SET DEBUG_SYNC='pcursor_move_to_next_block_latches_released SIGNAL latches_released WAIT_FOR go EXECUTE 2';
+# Send ALTER TABLE INPLACE to rebuild the index.
+SET SESSION debug="+d,pcursor_move_to_next_block_release_latches";
+ALTER TABLE t1 ENGINE=InnoDB, ALGORITHM=INPLACE;
+connection default;
+SET DEBUG_SYNC='now WAIT_FOR latches_released';
+SET DEBUG_SYNC='now SIGNAL go';
+SET DEBUG_SYNC='now WAIT_FOR latches_released';
+SET GLOBAL innodb_purge_run_now = ON;
+SET DEBUG_SYNC='now SIGNAL go';
+connection con1;
+# Reap ALTER TABLE.
+SET SESSION debug="-d,pcursor_move_to_next_block_release_latches";
+# Ensure all data is present after index rebuild.
+# Test cleanup.
+connection default;
+DROP TABLE t1;
+SET GLOBAL innodb_limit_optimistic_insert_debug=0;
 #
 # Cleanup.
+connection default;
 DROP TABLE IF EXISTS t1;
 Warnings:
 Note	1051	Unknown table 'test.t1'
+DROP PROCEDURE test.insert_records;
 disconnect con1;

mysql-test/suite/innodb/t/alter_table_rebuild_missing_record.test

@@ -4,10 +4,23 @@
 --source include/have_debug.inc
 --source include/have_debug_sync.inc
 
-SET DEBUG_SYNC= 'RESET';
 --enable_connect_log
 --connect (con1, localhost, root,,)
 
+--DELIMITER //
+
+CREATE PROCEDURE test.insert_records (IN record_count INT)
+BEGIN
+    DECLARE counter INT DEFAULT 1;
+
+    WHILE counter <= record_count DO
+        INSERT INTO test.t1 VALUES (0);
+        SET counter = counter + 1;
+    END WHILE;
+END //
+
+--DELIMITER ;
+
 --echo ##############################################################
 --echo # Bug#36846567 Inplace ALTER TABLE might cause lost rows if concurrent purge
 --echo ##############################################################
@@ -21,7 +34,7 @@ SET GLOBAL INNODB_PURGE_STOP_NOW=ON;
 DELETE FROM t1 WHERE pk = 'bbbcc';
 
 --connection con1
-SET DEBUG='+d,ddl_buf_add_two';
+SET SESSION DEBUG='+d,ddl_buf_add_two';
 SET DEBUG_SYNC='ddl_bulk_inserter_latches_released SIGNAL latches_released WAIT_FOR go';
 --echo # Send ALTER TABLE INPLACE which rebuilds table.
 --send ALTER TABLE t1 ENGINE=InnoDB, ALGORITHM=INPLACE
@@ -35,15 +48,13 @@ SET DEBUG_SYNC='now SIGNAL go';
 --connection con1
 --echo # Reap ALTER TABLE
 --reap
+SET SESSION DEBUG='-d,ddl_buf_add_two';
 
 --echo # Before the fix row 'ddddd' was missing from the table after ALTER.
 SELECT * FROM t1;
 
-SET DEBUG_SYNC= 'RESET';
-
 --echo # Test cleanup
 --connection default
-SET DEBUG_SYNC= 'RESET';
 DROP TABLE t1;
 
 --echo ##############################################################
@@ -60,7 +71,7 @@ SET GLOBAL INNODB_PURGE_STOP_NOW=ON;
 DELETE FROM t1 WHERE pk = 'aaaaa';
 
 --connection con1
-SET DEBUG='+d,ddl_buf_add_two';
+SET SESSION DEBUG='+d,ddl_buf_add_two';
 SET DEBUG_SYNC='ddl_bulk_inserter_latches_released SIGNAL latches_released WAIT_FOR go';
 --echo # Send ALTER TABLE INPLACE which rebuilds table.
 --send ALTER TABLE t1 ENGINE=InnoDB, ALGORITHM=INPLACE
@@ -74,15 +85,13 @@ SET DEBUG_SYNC='now SIGNAL go';
 --connection con1
 --echo # Reap ALTER TABLE
 --reap
+SET SESSION DEBUG='-d,ddl_buf_add_two';
 
 --echo # Verify intended records are present
 SELECT * FROM t1;
 
-SET DEBUG_SYNC= 'RESET';
-
 --echo # Test cleanup
 --connection default
-SET DEBUG_SYNC= 'RESET';
 DROP TABLE t1;
 
 --echo ##############################################################
@@ -98,7 +107,7 @@ SET GLOBAL INNODB_PURGE_STOP_NOW=ON;
 DELETE FROM t1 WHERE pk = 'bbbbb';
 
 --connection con1
-SET DEBUG='+d,ddl_buf_add_two';
+SET SESSION DEBUG='+d,ddl_buf_add_two';
 SET DEBUG_SYNC='ddl_bulk_inserter_latches_released SIGNAL latches_released WAIT_FOR go';
 --echo # Send ALTER TABLE INPLACE which rebuilds table.
 --send ALTER TABLE t1 ENGINE=InnoDB, ALGORITHM=INPLACE
@@ -112,15 +121,13 @@ SET DEBUG_SYNC='now SIGNAL go';
 --connection con1
 --echo # Reap ALTER TABLE
 --reap
+SET SESSION DEBUG='-d,ddl_buf_add_two';
 
 --echo # Verify intended records are present
 SELECT * FROM t1;
 
-SET DEBUG_SYNC= 'RESET';
-
 --echo # Test cleanup
 --connection default
-SET DEBUG_SYNC= 'RESET';
 DROP TABLE t1;
 
 --echo ##############################################################
@@ -136,7 +143,7 @@ SET GLOBAL INNODB_PURGE_STOP_NOW=ON;
 DELETE FROM t1 WHERE pk = 'ccccc';
 
 --connection con1
-SET DEBUG='+d,ddl_buf_add_two';
+SET SESSION DEBUG='+d,ddl_buf_add_two';
 SET DEBUG_SYNC='ddl_bulk_inserter_latches_released SIGNAL latches_released WAIT_FOR go';
 --echo # Send ALTER TABLE INPLACE which rebuilds table.
 --send ALTER TABLE t1 ENGINE=InnoDB, ALGORITHM=INPLACE
@@ -150,20 +157,88 @@ SET DEBUG_SYNC='now SIGNAL go';
 --connection con1
 --echo # Reap ALTER TABLE
 --reap
+SET SESSION DEBUG='-d,ddl_buf_add_two';
 
 --echo # Verify intended records are present
 SELECT * FROM t1;
 
-SET DEBUG_SYNC= 'RESET';
-
 --echo # Test cleanup
 --connection default
-SET DEBUG_SYNC= 'RESET';
 DROP TABLE t1;
 
+--echo ############################################################
+--echo # Test#5 Bug#37318367 Delete part of records from two pages
+--echo # to lose a record - pause in PCursor::move_to_next_block
+--echo ############################################################
+
+# Limit the max number of records per page to 3
+SET GLOBAL innodb_limit_optimistic_insert_debug=3;
+
+--connection default
+CREATE TABLE t1 (id int AUTO_INCREMENT NOT NULL, PRIMARY KEY(id)) ENGINE=InnoDB;
+
+# 8 records are inserted into four pages as following due to
+# sysvar innodb_limit_optimistic_insert_debug=3 set above :
+# p1: {1},  p2: {2,3,4}, p3: {5,6,7}, p4: {8}
+CALL insert_records(8);
+
+SET GLOBAL innodb_purge_stop_now = ON;
+
+#  Remove records in right half of page p2 and left half of page p3
+DELETE FROM t1 WHERE id > 2 AND id < 7;
+
+let $before_count = `SELECT COUNT(*) FROM t1`;
+--connection con1
+SET DEBUG_SYNC='pcursor_move_to_next_block_latches_released SIGNAL latches_released WAIT_FOR go EXECUTE 2';
+
+--echo # Send ALTER TABLE INPLACE to rebuild the index.
+
+# Trick the PCursor::move_to_next_block to think the latch is congested
+SET SESSION debug="+d,pcursor_move_to_next_block_release_latches";
+# Save the cursor position at the end of record#4 and wait for the second
+# 'go'  signal to resume operation.
+--send ALTER TABLE t1 ENGINE=InnoDB, ALGORITHM=INPLACE
+
+--connection default
+# wait for the first hit of the sync point at the end of first page ...
+SET DEBUG_SYNC='now WAIT_FOR latches_released';
+# ... which we ignore by immediately resuming the alter table operation
+SET DEBUG_SYNC='now SIGNAL go';
+# Now wait for the second hit of the sync point at the end of second page
+SET DEBUG_SYNC='now WAIT_FOR latches_released';
+SET GLOBAL innodb_purge_run_now = ON;
+--source include/wait_innodb_all_purged.inc
+
+# Resume the alter table operation after page p2 is processed. Since the purge
+# is completed so remaining records are arranged as following :
+# p1: {1, 2, 7}, p4: {8}
+# Without the fix, cursor is positioned to record#8 instead of record#7 thus
+# latter is skipped.
+SET DEBUG_SYNC='now SIGNAL go';
+
+--connection con1
+--echo # Reap ALTER TABLE.
+--reap;
+SET SESSION debug="-d,pcursor_move_to_next_block_release_latches";
+
+--echo # Ensure all data is present after index rebuild.
+let $after_count = `SELECT COUNT(*) FROM t1`;
+if($before_count != $after_count) {
+  --echo # Records, before rebuild=$before_count != after rebuild=$after_count"
+  SELECT id FROM t1;
+  --die "Test failed due to record count mismatch after index rebuild"
+}
+
+--echo # Test cleanup.
+--connection default
+DROP TABLE t1;
+SET GLOBAL innodb_limit_optimistic_insert_debug=0;
+
 --echo #
 --echo # Cleanup.
+--connection default
 DROP TABLE IF EXISTS t1;
+DROP PROCEDURE test.insert_records;
 --disconnect con1
 --disable_connect_log
 # Wait till all disconnects are completed