Bug#35115601 InnoDB - Page [...] still fixed or dirty crash during shutdown

Issue:
  During shutdown, after all internal threads are closed; it is
  expected that there are no pages in the buffer pool which are
  dirty or buffer fixed. During concurrent DDL and DML; the DDL
  will fail when the online log grows too big. When the DDL exits,
  it leaves some pages in a buffer fixed state. These pages remain
  buffer fixed in the buffer pool even after all internal threads
  are closed.

Fix:
  Error handling in the DDL must ensure that all pages buffer fixed
  by the DDL must be unfixed before the DDL exits.

Background:
  During the DDL, the tree is built using builders - one for each
  index. Each builder uses a set of page loaders to handle internal
  page operations. The builders and page loaders will buffer fix
  the pages they use, to ensure that these pages are not evicted
  before the builder completes. This is achieved via the functions
  latch() and release().
  The Page_load::release function will increase the buf_fix_count
  which is later decreased in the Page_load::latch function. Other
  page buffer fixing is tracked and released using MTR's mtr_memo_stack.

  In Builder::insert_direct, the pages at level 0 needs to be buffer
  fixed until the next call to insert_direct. This is done by the
  m_btr_load->release call before exit from insert_direct. In the
  next call to insert_direct, the m_btr_load->latch will "undo" the
  previous release call. The very first latch call will simply return
  as m_n_recs will be 0. So the order of latch and release will be:
    insert_direct(latch[], release[p0]),
    insert_direct(latch[p0], release[p1]),
    insert_direct(latch[p1], release[p2]) ...
  where p0, p1, p2.. are pages required for level 0. This order will
  continue until cursor.eof() returns true; after which we will do:
    insert_direct(latch[p(n-1)], release[pn]),
    insert_direct(latch[pn], m_btr_load->finish(DB_SUCCESS));

  Hence, when the Builder::insert_direct or the Builder::add_row is
  handling an error in the online build log, it must ensure to call
  m_btr_load->latch to buffer unfix page at level 0 and it must call
  m_btr_load->finish to handle the last pages of each page loader.

Change-Id: I956bef46b0d25fbad3b6eb92b197fe7023faf191

Author: ram1048

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

@@ -0,0 +1,22 @@
+# Bug scenario:
+CREATE TABLE t1 (c1 INT);
+# Create big enough table to ensure another call to Builder::insert_direct
+SELECT COUNT(*) FROM t1;
+COUNT(*)
+10001
+# Builder::insert_direct returns DB_ONLINE_LOG_TOO_BIG
+SET DEBUG="+d,builder_insert_direct_trigger_error";
+ALTER TABLE t1 ADD COLUMN c2 INT DEFAULT 20, ALGORITHM=INPLACE;
+ERROR HY000: Creating index 'GEN_CLUST_INDEX' required more than 'innodb_online_alter_log_max_size' bytes of modification log. Please try again.
+SET DEBUG="-d,builder_insert_direct_trigger_error";
+# Pages still buffer fixed should assert during shutdown
+# restart
+# Builder::add_row returns DB_ONLINE_LOG_TOO_BIG
+SET DEBUG="+d,builder_add_row_trigger_error";
+ALTER TABLE t1 ADD COLUMN c2 INT DEFAULT 20, ALGORITHM=INPLACE;
+ERROR HY000: Creating index 'GEN_CLUST_INDEX' required more than 'innodb_online_alter_log_max_size' bytes of modification log. Please try again.
+SET DEBUG="-d,builder_add_row_trigger_error";
+# Pages still buffer fixed should assert during shutdown
+# restart
+# Cleanup
+DROP TABLE t1;

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

@@ -0,0 +1,10 @@
+# Bug scenario:
+CREATE TABLE t1 (c1 INT);
+# Create big enough table to ensure another call to Builder::insert_direct
+SELECT COUNT(*) FROM t1;
+COUNT(*)
+10001
+SET DEBUG="+d,builder_insert_direct_no_builder";
+ALTER TABLE t1 ADD COLUMN c2 INT DEFAULT 20, ALGORITHM=INPLACE;
+ERROR HY000: Got error 11 - 'InnoDB error' from storage engine
+DROP TABLE t1;

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

@@ -0,0 +1,49 @@
+--source include/have_debug.inc
+# Bug scenario:
+# When DDL is on-going and hits an error in online build (for example when online log is too big); it must rollback
+# The error handling code has missed a call to m_btr_load->finish to cleanup the buffer fixed pages at each level
+
+--echo # Bug scenario:
+CREATE TABLE t1 (c1 INT);
+
+--echo # Create big enough table to ensure another call to Builder::insert_direct
+--disable_query_log
+DELIMITER |;
+CREATE PROCEDURE populate_t1(IN BASE INT, IN SIZE INT)
+BEGIN
+  DECLARE i INT DEFAULT BASE;
+  WHILE (i <= SIZE) DO
+    INSERT INTO t1 values (i);
+    SET i = i + 1;
+  END WHILE;
+END|
+DELIMITER ;|
+
+CALL populate_t1(0, 10000);
+DROP PROCEDURE populate_t1;
+--enable_query_log
+
+SELECT COUNT(*) FROM t1;
+
+--echo # Builder::insert_direct returns DB_ONLINE_LOG_TOO_BIG
+SET DEBUG="+d,builder_insert_direct_trigger_error";
+--error ER_INNODB_ONLINE_LOG_TOO_BIG
+ALTER TABLE t1 ADD COLUMN c2 INT DEFAULT 20, ALGORITHM=INPLACE;
+SET DEBUG="-d,builder_insert_direct_trigger_error";
+
+--echo # Pages still buffer fixed should assert during shutdown
+--source include/shutdown_mysqld.inc
+--source include/start_mysqld.inc
+
+--echo # Builder::add_row returns DB_ONLINE_LOG_TOO_BIG
+SET DEBUG="+d,builder_add_row_trigger_error";
+--error ER_INNODB_ONLINE_LOG_TOO_BIG
+ALTER TABLE t1 ADD COLUMN c2 INT DEFAULT 20, ALGORITHM=INPLACE;
+SET DEBUG="-d,builder_add_row_trigger_error";
+
+--echo # Pages still buffer fixed should assert during shutdown
+--source include/shutdown_mysqld.inc
+--source include/start_mysqld.inc
+
+--echo # Cleanup
+DROP TABLE t1;

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

@@ -0,0 +1,31 @@
+--source include/have_debug.inc
+# Test error handling in case m_btr_load was found nullptr
+
+--echo # Bug scenario:
+CREATE TABLE t1 (c1 INT);
+
+--echo # Create big enough table to ensure another call to Builder::insert_direct
+--disable_query_log
+call mtr.add_suppression("\\[ERROR\\] .* DDL Failed as Builder is already freed");
+DELIMITER |;
+CREATE PROCEDURE populate_t1(IN BASE INT, IN SIZE INT)
+BEGIN
+  DECLARE i INT DEFAULT BASE;
+  WHILE (i <= SIZE) DO
+    INSERT INTO t1 values (i);
+    SET i = i + 1;
+  END WHILE;
+END|
+DELIMITER ;|
+
+CALL populate_t1(0, 10000);
+DROP PROCEDURE populate_t1;
+--enable_query_log
+
+SELECT COUNT(*) FROM t1;
+
+SET DEBUG="+d,builder_insert_direct_no_builder";
+--error ER_GET_ERRNO
+ALTER TABLE t1 ADD COLUMN c2 INT DEFAULT 20, ALGORITHM=INPLACE;
+
+DROP TABLE t1;

share/messages_to_error_log.txt

@@ -12279,6 +12279,9 @@ ER_PERFORMANCE_SCHEMA_VERSION_CHANGE
 ER_WARN_DEPRECATED_OR_BLOCKED_CIPHER
    eng "Value for option '%s' contains cipher '%s' that is either blocked or deprecated (and will be removed in a future release). Please refer to the documentation for more details."
 
+ER_IB_MSG_DDL_FAIL_NO_BUILDER
+  eng "DDL Failed as Builder is already freed"
+
 # DO NOT add server-to-client messages here;
 # they go in messages_to_clients.txt
 # in the same directory as this file.

storage/innobase/btr/btr0load.cc

@@ -78,12 +78,14 @@ class Page_load : private ut::Non_copyable {
         m_page_no(page_no),
         m_level(level),
         m_is_comp(dict_table_is_comp(index->table)),
-        m_flush_observer(observer) {
+        m_flush_observer(observer),
+        m_n_blocks_buf_fixed(0) {
     ut_ad(!dict_index_is_spatial(m_index));
   }
 
   /** Destructor */
   ~Page_load() noexcept {
+    ut_a(m_n_blocks_buf_fixed == 0);
     if (m_heap != nullptr) {
       /* mtr is allocated using heap. */
       if (m_mtr != nullptr) {
@@ -285,6 +287,9 @@ class Page_load : private ut::Non_copyable {
   /** Page modified flag. */
   bool m_modified{};
 
+  /** Number of blocks which are buffer fixed but not pushed to mtr memo */
+  int32_t m_n_blocks_buf_fixed{};
+
   friend class Btree_load;
 };
 
@@ -810,6 +815,7 @@ void Page_load::release() noexcept {
 
   /* We fix the block because we will re-pin it soon. */
   buf_block_buf_fix_inc(m_block, UT_LOCATION_HERE);
+  m_n_blocks_buf_fixed++;
 
   m_modify_clock = m_block->get_modify_clock(IF_DEBUG(true));
 
@@ -851,6 +857,8 @@ void Page_load::latch() noexcept {
   }
 
   buf_block_buf_fix_dec(m_block);
+  m_n_blocks_buf_fixed--;
+  ut_a(m_n_blocks_buf_fixed >= 0);
 
   /* The caller is going to use the m_block, so it needs to be buffer-fixed
   even after the decrement above. This works like this:

storage/innobase/ddl/ddl0builder.cc

@@ -1217,6 +1217,23 @@ dberr_t Builder::key_buffer_sort(size_t thread_id) noexcept {
   return DB_SUCCESS;
 }
 
+dberr_t Builder::online_build_handle_error(dberr_t err) noexcept {
+  set_error(err);
+
+  if (m_btr_load != nullptr) {
+    /* page_loaders[0] has increased buf_fix_count through release(). This is
+    decremented by calling latch(). Similar release() calls for page_loaders at
+    non-zero levels are handled in finish() */
+    m_btr_load->latch();
+    err = m_btr_load->finish(err);
+
+    ut::delete_(m_btr_load);
+    m_btr_load = nullptr;
+  }
+
+  return get_error();
+}
+
 dberr_t Builder::insert_direct(Cursor &cursor, size_t thread_id) noexcept {
   ut_a(m_id == 0);
   ut_ad(is_skip_file_sort());
@@ -1229,15 +1246,28 @@ dberr_t Builder::insert_direct(Cursor &cursor, size_t thread_id) noexcept {
   {
     auto err = m_ctx.check_state_of_online_build_log();
 
+    DBUG_EXECUTE_IF("builder_insert_direct_trigger_error", {
+      static int count = 0;
+      ++count;
+      if (count > 1) {
+        err = DB_ONLINE_LOG_TOO_BIG;
+        m_ctx.m_trx->error_key_num = SERVER_CLUSTER_INDEX_ID;
+      }
+    });
+
     if (err != DB_SUCCESS) {
-      set_error(err);
-      err = m_btr_load->finish(err);
-      ut::delete_(m_btr_load);
-      m_btr_load = nullptr;
-      return get_error();
+      return online_build_handle_error(err);
     }
   }
 
+  DBUG_EXECUTE_IF("builder_insert_direct_no_builder",
+                  { static_cast<void>(online_build_handle_error(DB_ERROR)); });
+
+  if (m_btr_load == nullptr) {
+    ib::error(ER_IB_MSG_DDL_FAIL_NO_BUILDER);
+    return DB_ERROR;
+  }
+
   m_btr_load->latch();
 
   auto thread_ctx = m_thread_ctxs[thread_id];
@@ -1544,8 +1574,13 @@ dberr_t Builder::add_row(Cursor &cursor, Row &row, size_t thread_id,
                          Latch_release &&latch_release) noexcept {
   auto err = m_ctx.check_state_of_online_build_log();
 
+  DBUG_EXECUTE_IF("builder_add_row_trigger_error", {
+    err = DB_ONLINE_LOG_TOO_BIG;
+    m_ctx.m_trx->error_key_num = SERVER_CLUSTER_INDEX_ID;
+  });
+
   if (err != DB_SUCCESS) {
-    set_error(err);
+    err = online_build_handle_error(err);
   } else if (is_spatial_index()) {
     if (!cursor.eof()) {
       err = batch_add_row(row, thread_id);

storage/innobase/include/ddl0impl-builder.h

@@ -395,6 +395,16 @@ struct Builder {
   [[nodiscard]] dberr_t check_duplicates(Thread_ctxs &dupcheck,
                                          Dup *dup) noexcept;
 
+  /** Cleanup DDL after error in online build
+  Note: To be called if DDL must cleanup due to error in online build. Pages
+  which are buffer-fixed (in Page_load::release) until the next iteration, must
+  be unfixed (with Page_load::latch) before returning the error.
+  @note: Assumes that either m_btr_load->release is called before or
+  m_n_recs is 0 (no records are inserted yet).
+  @param[in]  err    Error hit in online build
+  @return the cursor error status. */
+  [[nodiscard]] dberr_t online_build_handle_error(dberr_t err) noexcept;
+
  private:
   /** Buffer ID. */
   size_t m_id{};