Bug#37318367 Inplace ALTER on table with spatial idx might cause lost rows if concurrent purge

Description:
------------
A table with a secondary index on a spatial column may lose records
during an inplace ALTER TABLE operation if the records are deleted
and purged concurrently. This issue is specific to spatial indexes
and is similar to Bug#36846567, which was reported for tables with a
clustered index.

Analysis:
---------
During the inplace ALTER TABLE rebuild, the records in the clustered
index are scanned with the help of a cursor. Cursor tracks the record
position in the index. If the table has spatial index(es) then,
during the scan, its respective builder instance caches the records in
a tuple vector using the method Builder::batch_add_row().
batch_add_row() is called through a bulk_inserter lambda.
Once the cursor reaches at the end of the page, the cached tuples are
inserted (processed) into the spatial index(es) using the method
Builder::batch_insert(). batch_insert() is called through batch_inserter
lambda.
While processing the records, if the batch_inserter detects that the
server is running out of space in redo logs, it deep copies the
remaining user records in the tuple vector of current builder and from
the remaining other builders of spatial index(es) as well.
Batch_inserter than calls the savepoint() that first backs off the
cursor by one position to point to a user record, saves that new
position and, then releases the latch(es) held by the mini transaction.

When the scan resumes, cursor must be positioned to either, the user
record which comes next after the position saved before or, if there
is no such record, it must restore to supremum on the last leaf page.

At present PCursor::restore_from_savepoint() calls resume() to restore
the cursor position.

In the scenario reported, records are purged and the remaining records
are rearranged in the index. Here is what happens :

1. resume() calls PCursor::restore_position() which is wrapper over the
btr_pcur_t::restore_position().

2: btr_pcur_t::restore_position first restores the position on the
record saved before or its predecessor and returns false because, it is
not original record.

3: Then the caller of step#2, PCursor::restore_position() detects
the returned value false. It decides to move to the next record.

4: Finally, the caller of step#3, PCursor::resume() notices that cursor
is positioned on the last record of the page, then it moves the cursor
to the successor record.

Notice step#4 is assuming that step#3 restores the cursor to the saved
record(or its predecessor) but that is not case, as a result the record
where the cursor was positioned at step#3 is lost, how exactly?
Actually after step#3, the cursor is either
(a) positioned on the saved record, or if it was removed meanwhile:
(b) on a user record which comes after it or,
(c) a supremum record which comes after the hole which it left.
The logic in PCursor::resume() correctly handles case (a) by advancing,
case (c) by doing nothing, but fails in case of (b), as it advances the
cursor needlessly.

Fix:
----
As part of the Bug#36846567 fix, it was identified that the current
PCursor wrapper APIs attempted to handle multiple scenarios, making it
difficult to implement them correctly. The decision was made to separate
the APIs so that each performs a specific task neatly.

This fix introduces a similar set of save/restore APIs for the batch
inserter mode used for R-Tree index(es) scan:

- save_previous_user_record_as_last_processed:
  This save API stores the position of the last user record on the page.

- restore_to_first_unprocessed:
  This restore API positions the cursor to the record which comes next
  after the cursor position saved by the paired save API.

Change-Id: I89627e7904293dcc58ccbb8b8a64f73b56e8dc93

Author: RahulSisondia

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

@@ -0,0 +1,43 @@
+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 (srid)
+VALUES (ST_PointFromText('POINT(1 1)'));
+SET counter = counter + 1;
+END WHILE;
+END //
+SET GLOBAL innodb_limit_optimistic_insert_debug=3;
+############################################################
+# Test#1 Bug#37318367 Delete part of records from two pages
+# to lose a record
+############################################################
+connection default;
+CREATE TABLE t1 (id int AUTO_INCREMENT NOT NULL, srid POINT NOT NULL SRID 0,
+PRIMARY KEY(id), SPATIAL INDEX spi(srid)) 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='ddl_batch_inserter_latches_released SIGNAL latches_released WAIT_FOR go EXECUTE 2';
+# Send ALTER TABLE INPLACE to rebuild the index.
+SET SESSION debug="+d,ddl_instrument_log_check_flush";
+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,ddl_instrument_log_check_flush";
+# Ensure all data is present after index rebuild.
+# Test cleanup.
+DROP TABLE t1;
+# cleanup
+connection default;
+DROP PROCEDURE test.insert_records;
+SET GLOBAL innodb_limit_optimistic_insert_debug=0;
+disconnect con1;

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

@@ -0,0 +1,99 @@
+# Save the initial number of concurrent sessions
+--source include/count_sessions.inc
+--source include/have_debug.inc
+--source include/have_debug_sync.inc
+
+--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 (srid)
+          VALUES (ST_PointFromText('POINT(1 1)'));
+        SET counter = counter + 1;
+    END WHILE;
+END //
+
+--DELIMITER ;
+
+# Limit the max number of records per page to 3
+SET GLOBAL innodb_limit_optimistic_insert_debug=3;
+
+--echo ############################################################
+--echo # Test#1 Bug#37318367 Delete part of records from two pages
+--echo # to lose a record
+--echo ############################################################
+
+--connection default
+CREATE TABLE t1 (id int AUTO_INCREMENT NOT NULL, srid POINT NOT NULL SRID 0,
+  PRIMARY KEY(id), SPATIAL INDEX spi(srid)) 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='ddl_batch_inserter_latches_released SIGNAL latches_released WAIT_FOR go EXECUTE 2';
+
+--echo # Send ALTER TABLE INPLACE to rebuild the index.
+
+# Force the log free check so that latch could be released at the end of the page
+SET SESSION debug="+d,ddl_instrument_log_check_flush";
+
+# 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,ddl_instrument_log_check_flush";
+
+--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.
+DROP TABLE t1;
+
+--echo # cleanup
+--connection default
+DROP PROCEDURE test.insert_records;
+SET GLOBAL innodb_limit_optimistic_insert_debug=0;
+--disconnect con1
+# Wait till all disconnects are completed
+--source include/wait_until_count_sessions.inc

storage/innobase/ddl/ddl0par-scan.cc

@@ -205,8 +205,9 @@ dberr_t Parallel_cursor::scan(Builders &builders) noexcept {
           for (size_t j = i + 1; j < batch_insert.size(); ++j) {
             batch_insert[j]->batch_insert_deep_copy_tuples(thread_id);
           }
-          thread_ctx->savepoint();
+          thread_ctx->save_previous_user_record_as_last_processed();
           latches_released = true;
+          DEBUG_SYNC_C("ddl_batch_inserter_latches_released");
         }
         return DB_SUCCESS;
       });
@@ -219,7 +220,8 @@ dberr_t Parallel_cursor::scan(Builders &builders) noexcept {
     }
 
     if (latches_released) {
-      return thread_ctx->restore_from_savepoint();
+      /* Resume from the savepoint (above). */
+      thread_ctx->restore_to_first_unprocessed();
     }
 
     return DB_SUCCESS;

storage/innobase/include/row0pread.h

@@ -257,12 +257,18 @@ class Parallel_reader {
     @return DB_SUCCESS or error code. */
     [[nodiscard]] dberr_t restore_from_savepoint() noexcept;
 
-    /** Save the current user record position, commit any active mtr. */
+    /** @see PCursor::save_current_user_record_as_last_processed */
     void save_current_user_record_as_last_processed() noexcept;
 
-    /** Restore the saved user record position to resume the scan */
+    /** @see PCursor::restore_to_last_processed_user_record */
     void restore_to_last_processed_user_record() noexcept;
 
+    /** @see PCursor::save_previous_user_record_as_last_processed */
+    void save_previous_user_record_as_last_processed() noexcept;
+
+    /** @see PCursor::restore_to_first_unprocessed */
+    void restore_to_first_unprocessed() noexcept;
+
     /** Thread ID. */
     size_t m_thread_id{std::numeric_limits<size_t>::max()};
 

storage/innobase/row/row0pread.cc

@@ -232,9 +232,25 @@ class PCursor {
   record. If all records were removed then restore the cursor to the infimum;
   this must be fine if the next step of the caller is to advance the cursor.
   @note This method must be paired with
-  save_current_user_record_as_last_processed() to restore the record position */
+  save_current_user_record_as_last_processed() to save the record position */
   void restore_to_last_processed_user_record() noexcept;
 
+  /** This method must be called after all records on a page are processed and
+  cursor is positioned at supremum. Under this assumption, it stores the
+  position BTR_PCUR_AFTER the last user record on the page.
+  This method must be paired with restore_to_first_unprocessed() to restore to
+  a record which comes right after the value of the stored last processed
+  record @see restore_to_first_unprocessed for details. */
+  void save_previous_user_record_as_last_processed() noexcept;
+
+  /** Restore the cursor to the record which comes next after the saved
+  position by the paired method save_previous_user_record_as_last_processed().
+  If there is no such record, it will restore to supremum on the last leaf
+  page.
+  @note This method must be paired with
+  save_previous_user_record_as_last_processed() to save the record position */
+  void restore_to_first_unprocessed() noexcept;
+
   /** Move to the next block.
   @param[in]  index             Index being traversed.
   @return DB_SUCCESS or error code. */
@@ -403,6 +419,29 @@ void PCursor::restore_to_last_processed_user_record() noexcept {
   ut_ad(same_row);
 }
 
+void PCursor::save_previous_user_record_as_last_processed() noexcept {
+  ut_a(m_pcur->is_after_last_on_page());
+  m_pcur->store_position(m_mtr);
+  ut_a(m_pcur->m_rel_pos == BTR_PCUR_AFTER);
+  m_mtr->commit();
+}
+
+void PCursor::restore_to_first_unprocessed() noexcept {
+  ut_a(m_pcur->m_rel_pos == BTR_PCUR_AFTER);
+  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 {
   auto cur = m_pcur->get_page_cur();
   const auto next_page_no = btr_page_get_next(page_cur_get_page(cur), m_mtr);
@@ -479,6 +518,15 @@ void Parallel_reader::Thread_ctx::
   m_pcursor->restore_to_last_processed_user_record();
 }
 
+void Parallel_reader::Thread_ctx::
+    save_previous_user_record_as_last_processed() noexcept {
+  m_pcursor->save_previous_user_record_as_last_processed();
+}
+
+void Parallel_reader::Thread_ctx::restore_to_first_unprocessed() noexcept {
+  m_pcursor->restore_to_first_unprocessed();
+}
+
 dberr_t PCursor::move_to_next_block(dict_index_t *index) {
   ut_ad(m_pcur->is_after_last_on_page());
   dberr_t err = DB_SUCCESS;