MDEV-36122 Assertion ctx0->old_table->get_ref_count() == 1

trx_purge_close_tables(): Before releasing any metadata locks (MDL),
release all table references, in case an ALTER TABLE…ALGORITHM=COPY
operation has confused our logic.

trx_purge_table_acquire(), trx_purge_table_open(): Do not acquire any
table reference before successfully acquiring a necessary metadata lock.
In this way, if purge is waiting for MDL, a concurrent
ha_innobase::commit_inplace_alter_table(commit=true) that is holding
a conflicting MDL_EXCLUSIVE will only observe its own reference on
the table that it may need to replace.

dict_acquire_mdl_shared<false>(): Unless we hold an MDL or one is
not needed, do not hold a table reference when releasing dict_sys.latch.
After loading a table into the dictionary cache, we will look up the
table again, because the table could be evicted or dropped while we
were not holding any dict_sys.latch.

Reviewed by: Debarun Banerjee

Author: dr-m

storage/innobase/dict/dict0dict.cc

@@ -694,35 +694,37 @@ dict_acquire_mdl_shared(dict_table_t *table,
                         MDL_context *mdl_context, MDL_ticket **mdl,
                         dict_table_op_t table_op)
 {
-  table_id_t table_id= table->id;
   char db_buf[NAME_LEN + 1], db_buf1[NAME_LEN + 1];
   char tbl_buf[NAME_LEN + 1], tbl_buf1[NAME_LEN + 1];
   size_t db_len, tbl_len;
-  bool unaccessible= false;
 
   if (!table->parse_name<!trylock>(db_buf, tbl_buf, &db_len, &tbl_len))
     /* The name of an intermediate table starts with #sql */
     return table;
 
 retry:
-  if (!unaccessible && (!table->is_readable() || table->corrupted))
+  ut_ad(!trylock == dict_sys.frozen());
+  ut_ad(trylock || table->get_ref_count());
+
+  if (!table->is_readable() || table->corrupted)
   {
+    if (!trylock)
+      table->release();
     if (*mdl)
     {
       mdl_context->release_lock(*mdl);
       *mdl= nullptr;
     }
-    unaccessible= true;
+    return nullptr;
   }
 
-  if (!trylock)
-    table->release();
-
-  if (unaccessible)
-    return nullptr;
+  const table_id_t table_id{table->id};
 
   if (!trylock)
+  {
+    table->release();
     dict_sys.unfreeze();
+  }
 
   {
     MDL_request request;
@@ -748,43 +750,53 @@ dict_acquire_mdl_shared(dict_table_t *table,
     }
   }
 
+  size_t db1_len, tbl1_len;
+lookup:
   dict_sys.freeze(SRW_LOCK_CALL);
   table= dict_sys.find_table(table_id);
   if (table)
+  {
+    if (!table->is_accessible())
+    {
+      table= nullptr;
+    unlock_and_return_without_mdl:
+      if (trylock)
+        dict_sys.unfreeze();
+    return_without_mdl:
+      if (*mdl)
+      {
+        mdl_context->release_lock(*mdl);
+        *mdl= nullptr;
+      }
+      return table;
+    }
+
     table->acquire();
-  if (!table && table_op != DICT_TABLE_OP_OPEN_ONLY_IF_CACHED)
+
+    if (!table->parse_name<true>(db_buf1, tbl_buf1, &db1_len, &tbl1_len))
+    {
+      /* The table was renamed to #sql prefix.
+      Release MDL (if any) for the old name and return. */
+      goto unlock_and_return_without_mdl;
+    }
+  }
+  else if (table_op != DICT_TABLE_OP_OPEN_ONLY_IF_CACHED)
   {
     dict_sys.unfreeze();
     dict_sys.lock(SRW_LOCK_CALL);
     table= dict_load_table_on_id(table_id,
                                  table_op == DICT_TABLE_OP_LOAD_TABLESPACE
                                  ? DICT_ERR_IGNORE_RECOVER_LOCK
                                  : DICT_ERR_IGNORE_FK_NOKEY);
-    if (table)
-      table->acquire();
     dict_sys.unlock();
-    dict_sys.freeze(SRW_LOCK_CALL);
-  }
-
-  if (!table || !table->is_accessible())
-  {
-return_without_mdl:
-    if (trylock)
-      dict_sys.unfreeze();
-    if (*mdl)
-    {
-      mdl_context->release_lock(*mdl);
-      *mdl= nullptr;
-    }
-    return nullptr;
-  }
-
-  size_t db1_len, tbl1_len;
-
-  if (!table->parse_name<true>(db_buf1, tbl_buf1, &db1_len, &tbl1_len))
-  {
-    /* The table was renamed to #sql prefix.
-    Release MDL (if any) for the old name and return. */
+    /* At this point, the freshly loaded table may already have been evicted.
+    We must look it up again while holding a shared dict_sys.latch.  We keep
+    trying this until the table is found in the cache or it cannot be found
+    in the dictionary (because the table has been dropped or rebuilt). */
+    if (table)
+      goto lookup;
+    if (!trylock)
+      dict_sys.freeze(SRW_LOCK_CALL);
     goto return_without_mdl;
   }
 

storage/innobase/trx/trx0purge.cc

@@ -1051,16 +1051,25 @@ inline trx_purge_rec_t purge_sys_t::fetch_next_rec()
 /** Close all tables that were opened in a purge batch for a worker.
 @param node   purge task context
 @param thd    purge coordinator thread handle */
-static void trx_purge_close_tables(purge_node_t *node, THD *thd)
+static void trx_purge_close_tables(purge_node_t *node, THD *thd) noexcept
 {
   for (auto &t : node->tables)
   {
-    if (!t.second.first);
-    else if (t.second.first == reinterpret_cast<dict_table_t*>(-1));
-    else
+    dict_table_t *table= t.second.first;
+    if (table != nullptr && table != reinterpret_cast<dict_table_t*>(-1))
+      dict_table_close(table);
+  }
+
+  MDL_context *mdl_context= static_cast<MDL_context*>(thd_mdl_context(thd));
+
+  for (auto &t : node->tables)
+  {
+    dict_table_t *table= t.second.first;
+    if (table != nullptr && table != reinterpret_cast<dict_table_t*>(-1))
     {
-      dict_table_close(t.second.first, false, thd, t.second.second);
       t.second.first= reinterpret_cast<dict_table_t*>(-1);
+      if (mdl_context != nullptr && t.second.second != nullptr)
+        mdl_context->release_lock(t.second.second);
     }
   }
 }
@@ -1072,44 +1081,43 @@ void purge_sys_t::wait_FTS(bool also_sys)
 }
 
 __attribute__((nonnull))
-/** Aqcuire a metadata lock on a table.
+/** Acquire a metadata lock on a table.
 @param table        table handle
 @param mdl_context  metadata lock acquisition context
-@param mdl          metadata lcok
+@param mdl          metadata lock
 @return table handle
 @retval nullptr if the table is not found or accessible
 @retval -1      if the purge of history must be suspended due to DDL */
 static dict_table_t *trx_purge_table_acquire(dict_table_t *table,
                                              MDL_context *mdl_context,
-                                             MDL_ticket **mdl)
+                                             MDL_ticket **mdl) noexcept
 {
   ut_ad(dict_sys.frozen_not_locked());
   *mdl= nullptr;
 
   if (!table->is_readable() || table->corrupted)
-  {
-    table->release();
     return nullptr;
-  }
 
   size_t db_len= dict_get_db_name_len(table->name.m_name);
   if (db_len == 0)
-    return table; /* InnoDB system tables are not covered by MDL */
+  {
+    /* InnoDB system tables are not covered by MDL */
+  got_table:
+    table->acquire();
+    return table;
+  }
 
   if (purge_sys.must_wait_FTS())
-  {
   must_wait:
-    table->release();
     return reinterpret_cast<dict_table_t*>(-1);
-  }
 
   char db_buf[NAME_LEN + 1];
   char tbl_buf[NAME_LEN + 1];
   size_t tbl_len;
 
   if (!table->parse_name<true>(db_buf, tbl_buf, &db_len, &tbl_len))
     /* The name of an intermediate table starts with #sql */
-    return table;
+    goto got_table;
 
   {
     MDL_request request;
@@ -1122,37 +1130,38 @@ static dict_table_t *trx_purge_table_acquire(dict_table_t *table,
       goto must_wait;
   }
 
-  return table;
+  goto got_table;
 }
 
 /** Open a table handle for the purge of committed transaction history
 @param table_id     InnoDB table identifier
 @param mdl_context  metadata lock acquisition context
-@param mdl          metadata lcok
+@param mdl          metadata lock
 @return table handle
 @retval nullptr if the table is not found or accessible
 @retval -1      if the purge of history must be suspended due to DDL */
 static dict_table_t *trx_purge_table_open(table_id_t table_id,
                                           MDL_context *mdl_context,
-                                          MDL_ticket **mdl)
+                                          MDL_ticket **mdl) noexcept
 {
-  dict_sys.freeze(SRW_LOCK_CALL);
-
-  dict_table_t *table= dict_sys.find_table(table_id);
+  dict_table_t *table;
 
-  if (table)
-    table->acquire();
-  else
+  for (;;)
   {
+    dict_sys.freeze(SRW_LOCK_CALL);
+    table= dict_sys.find_table(table_id);
+    if (table)
+      break;
     dict_sys.unfreeze();
     dict_sys.lock(SRW_LOCK_CALL);
     table= dict_load_table_on_id(table_id, DICT_ERR_IGNORE_FK_NOKEY);
-    if (table)
-      table->acquire();
     dict_sys.unlock();
     if (!table)
       return nullptr;
-    dict_sys.freeze(SRW_LOCK_CALL);
+    /* At this point, the freshly loaded table may already have been evicted.
+    We must look it up again while holding a shared dict_sys.latch.  We keep
+    trying this until the table is found in the cache or it cannot be found
+    in the dictionary (because the table has been dropped or rebuilt). */
   }
 
   table= trx_purge_table_acquire(table, mdl_context, mdl);
@@ -1171,10 +1180,7 @@ dict_table_t *purge_sys_t::close_and_reopen(table_id_t id, THD *thd,
 
   for (que_thr_t *thr= UT_LIST_GET_FIRST(purge_sys.query->thrs); thr;
        thr= UT_LIST_GET_NEXT(thrs, thr))
-  {
-    purge_node_t *node= static_cast<purge_node_t*>(thr->child);
-    trx_purge_close_tables(node, thd);
-  }
+    trx_purge_close_tables(static_cast<purge_node_t*>(thr->child), thd);
 
   m_active= false;
   wait_FTS(false);