MDEV-36122: Protect table references with a lock

dict_table_open_on_id(): Simplify the logic. dict_stats: A helper for acquiring MDL and opening the tables mysql.innodb_table_stats and mysql.innodb_index_stats. innodb_ft_aux_table_validate(): Contiguously hold dict_sys.latch while accessing the table that we open with dict_table_open_on_name(). lock_table_children(): Do not hold a table reference while invoking dict_acquire_mdl_shared<false>(), which may temporarily release and reacquire the shared dict_sys.latch that we are holding. prepare_inplace_alter_table_dict(): If an unexpected reference to the table exists, wait for the purge subsystem to release its table handle, similar to how we would do in case FULLTEXT INDEX existed. This function is supposed to be protected by MDL_EXCLUSIVE on the table name. If purge is going to access the table again later during is ALTER TABLE operation, it will have access to an MDL compatible name for it and therefore should conflict with any MDL_EXCLUSIVE that would cover ha_innobase::commit_inplace_alter_table(commit=true). ha_innobase::rename_table(): Before locking the data dictionary, ensure that the purge subsystem is not holding a reference to the table due to the lack of metadata locking, related to FULLTEXT INDEX or the row-level undo logging of ALTER IGNORE TABLE. ha_innobase::truncate(): Before locking the data dictionary, ensure that the purge subsystem is not holding a reference to the table due to insufficient metadata locking related to an earlier ALTER IGNORE TABLE operation. trx_purge_attach_undo_recs(), purge_sys_t::batch_cleanup(): Clear purge_sys.m_active only after all table handles have been released. With these changes, no caller of dict_acquire_mdl_shared<false> should be holding a table reference. All remaining calls to dict_table_open_on_name(dict_locked=false) except the one in fts_lock_table() and possibly in the DDL recovery predicate innodb_check_version() should be protected by MDL, but there currently is no assertion that would enforce this. Reviewed by: Debarun Banerjee

Author: dr-m

storage/innobase/dict/dict0defrag_bg.cc

@@ -217,47 +217,17 @@ dberr_t dict_stats_save_defrag_summary(dict_index_t *index, THD *thd)
  if (index->is_ibuf())
  return DB_SUCCESS;
 
- MDL_ticket *mdl_table= nullptr, *mdl_index= nullptr;
- dict_table_t *table_stats= dict_table_open_on_name(TABLE_STATS_NAME, false,
- DICT_ERR_IGNORE_NONE);
- if (table_stats)
- {
- dict_sys.freeze(SRW_LOCK_CALL);
- table_stats= dict_acquire_mdl_shared<false>(table_stats, thd, &mdl_table);
- dict_sys.unfreeze();
- }
- if (!table_stats || strcmp(table_stats->name.m_name, TABLE_STATS_NAME))
- {
-release_and_exit:
- if (table_stats)
- dict_table_close(table_stats, thd, mdl_table);
+ dict_stats stats;
+ if (stats.open(thd))
  return DB_STATS_DO_NOT_EXIST;
- }
-
- dict_table_t *index_stats= dict_table_open_on_name(INDEX_STATS_NAME, false,
- DICT_ERR_IGNORE_NONE);
- if (index_stats)
- {
- dict_sys.freeze(SRW_LOCK_CALL);
- index_stats= dict_acquire_mdl_shared<false>(index_stats, thd, &mdl_index);
- dict_sys.unfreeze();
- }
- if (!index_stats)
- goto release_and_exit;
- if (strcmp(index_stats->name.m_name, INDEX_STATS_NAME))
- {
- dict_table_close(index_stats, thd, mdl_index);
- goto release_and_exit;
- }
-
  trx_t *trx= trx_create();
  trx->mysql_thd= thd;
  trx_start_internal(trx);
  dberr_t ret= trx->read_only
  ? DB_READ_ONLY
- : lock_table_for_trx(table_stats, trx, LOCK_X);
+ : lock_table_for_trx(stats.table(), trx, LOCK_X);
  if (ret == DB_SUCCESS)
- ret= lock_table_for_trx(index_stats, trx, LOCK_X);
+ ret= lock_table_for_trx(stats.index(), trx, LOCK_X);
  row_mysql_lock_data_dictionary(trx);
  if (ret == DB_SUCCESS)
  ret= dict_stats_save_index_stat(index, time(nullptr), "n_pages_freed",
@@ -271,13 +241,9 @@ dberr_t dict_stats_save_defrag_summary(dict_index_t *index, THD *thd)
  else
  trx->rollback();
 
- if (table_stats)
- dict_table_close(table_stats, thd, mdl_table);
- if (index_stats)
- dict_table_close(index_stats, thd, mdl_index);
-
  row_mysql_unlock_data_dictionary(trx);
  trx->free();
+ stats.close();
 
  return ret;
 }
@@ -353,49 +319,18 @@ dict_stats_save_defrag_stats(
  if (!n_leaf_reserved)
  return DB_SUCCESS;
 
- THD *thd= current_thd;
- MDL_ticket *mdl_table= nullptr, *mdl_index= nullptr;
- dict_table_t* table_stats= dict_table_open_on_name(TABLE_STATS_NAME, false,
- DICT_ERR_IGNORE_NONE);
- if (table_stats)
- {
- dict_sys.freeze(SRW_LOCK_CALL);
- table_stats= dict_acquire_mdl_shared<false>(table_stats, thd, &mdl_table);
- dict_sys.unfreeze();
- }
- if (!table_stats || strcmp(table_stats->name.m_name, TABLE_STATS_NAME))
- {
-release_and_exit:
- if (table_stats)
- dict_table_close(table_stats, thd, mdl_table);
+ THD *const thd= current_thd;
+ dict_stats stats;
+ if (stats.open(thd))
  return DB_STATS_DO_NOT_EXIST;
- }
-
- dict_table_t *index_stats= dict_table_open_on_name(INDEX_STATS_NAME, false,
- DICT_ERR_IGNORE_NONE);
- if (index_stats)
- {
- dict_sys.freeze(SRW_LOCK_CALL);
- index_stats= dict_acquire_mdl_shared<false>(index_stats, thd, &mdl_index);
- dict_sys.unfreeze();
- }
- if (!index_stats)
- goto release_and_exit;
-
- if (strcmp(index_stats->name.m_name, INDEX_STATS_NAME))
- {
- dict_table_close(index_stats, thd, mdl_index);
- goto release_and_exit;
- }
-
  trx_t *trx= trx_create();
  trx->mysql_thd= thd;
  trx_start_internal(trx);
  dberr_t ret= trx->read_only
  ? DB_READ_ONLY
- : lock_table_for_trx(table_stats, trx, LOCK_X);
+ : lock_table_for_trx(stats.table(), trx, LOCK_X);
  if (ret == DB_SUCCESS)
- ret= lock_table_for_trx(index_stats, trx, LOCK_X);
+ ret= lock_table_for_trx(stats.index(), trx, LOCK_X);
 
  row_mysql_lock_data_dictionary(trx);
 
@@ -423,12 +358,9 @@ dict_stats_save_defrag_stats(
  else
  trx->rollback();
 
- if (table_stats)
- dict_table_close(table_stats, thd, mdl_table);
- if (index_stats)
- dict_table_close(index_stats, thd, mdl_index);
  row_mysql_unlock_data_dictionary(trx);
  trx->free();
+ stats.close();
 
  return ret;
 }

storage/innobase/dict/dict0dict.cc

@@ -647,12 +647,9 @@ dict_acquire_mdl_shared(dict_table_t *table,
 
 retry:
  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);
@@ -664,10 +661,7 @@ dict_acquire_mdl_shared(dict_table_t *table,
  const table_id_t table_id{table->id};
 
  if (!trylock)
- {
- table->release();
  dict_sys.unfreeze();
- }
 
  {
  MDL_request request;
@@ -714,7 +708,8 @@ dict_acquire_mdl_shared(dict_table_t *table,
  return table;
  }
 
- table->acquire();
+ if (trylock)
+ table->acquire();
 
  if (!table->parse_name<true>(db_buf1, tbl_buf1, &db1_len, &tbl1_len))
  {
@@ -828,16 +823,29 @@ dict_table_t *dict_table_open_on_id(table_id_t table_id, bool dict_locked,
  dict_table_op_t table_op, THD *thd,
  MDL_ticket **mdl)
 {
+retry:
  if (!dict_locked)
  dict_sys.freeze(SRW_LOCK_CALL);
 
  dict_table_t *table= dict_sys.find_table(table_id);
 
  if (table)
  {
- table->acquire();
- if (thd && !dict_locked)
- table= dict_acquire_mdl_shared<false>(table, thd, mdl, table_op);
+ if (!dict_locked)
+ {
+ if (thd)
+ {
+ table= dict_acquire_mdl_shared<false>(table, thd, mdl, table_op);
+ if (table)
+ goto acquire;
+ }
+ else
+ acquire:
+ table->acquire();
+ dict_sys.unfreeze();
+ }
+ else
+ table->acquire();
  }
  else if (table_op != DICT_TABLE_OP_OPEN_ONLY_IF_CACHED)
  {
@@ -850,24 +858,16 @@ dict_table_t *dict_table_open_on_id(table_id_t table_id, bool dict_locked,
  table_op == DICT_TABLE_OP_LOAD_TABLESPACE
  ? DICT_ERR_IGNORE_RECOVER_LOCK
  : DICT_ERR_IGNORE_FK_NOKEY);
- if (table)
- table->acquire();
  if (!dict_locked)
  {
  dict_sys.unlock();
- if (table && thd)
- {
- dict_sys.freeze(SRW_LOCK_CALL);
- table= dict_acquire_mdl_shared<false>(table, thd, mdl, table_op);
- dict_sys.unfreeze();
- }
- return table;
+ if (table)
+ goto retry;
  }
+ else if (table)
+ table->acquire();
  }
 
- if (!dict_locked)
- dict_sys.unfreeze();
-
  return table;
 }
 
@@ -1084,6 +1084,55 @@ dict_table_open_on_name(
  DBUG_RETURN(table);
 }
 
+bool dict_stats::open(THD *thd) noexcept
+{
+ ut_ad(!mdl_table);
+ ut_ad(!mdl_index);
+ ut_ad(!table_stats);
+ ut_ad(!index_stats);
+ ut_ad(!mdl_context);
+
+ mdl_context= static_cast<MDL_context*>(thd_mdl_context(thd));
+ if (!mdl_context)
+ return true;
+ /* FIXME: use compatible type, and maybe remove this parameter altogether! */
+ const double timeout= double(global_system_variables.lock_wait_timeout);
+ MDL_request request;
+ MDL_REQUEST_INIT(&request, MDL_key::TABLE, "mysql", "innodb_table_stats",
+ MDL_SHARED, MDL_EXPLICIT);
+ if (UNIV_UNLIKELY(mdl_context->acquire_lock(&request, timeout)))
+ return true;
+ mdl_table= request.ticket;
+ MDL_REQUEST_INIT(&request, MDL_key::TABLE, "mysql", "innodb_index_stats",
+ MDL_SHARED, MDL_EXPLICIT);
+ if (UNIV_UNLIKELY(mdl_context->acquire_lock(&request, timeout)))
+ goto release_mdl;
+ mdl_index= request.ticket;
+ table_stats= dict_table_open_on_name("mysql/innodb_table_stats", false,
+ DICT_ERR_IGNORE_NONE);
+ if (!table_stats)
+ goto release_mdl;
+ index_stats= dict_table_open_on_name("mysql/innodb_index_stats", false,
+ DICT_ERR_IGNORE_NONE);
+ if (index_stats)
+ return false;
+
+ table_stats->release();
+release_mdl:
+ if (mdl_index)
+ mdl_context->release_lock(mdl_index);
+ mdl_context->release_lock(mdl_table);
+ return true;
+}
+
+void dict_stats::close() noexcept
+{
+ table_stats->release();
+ index_stats->release();
+ mdl_context->release_lock(mdl_table);
+ mdl_context->release_lock(mdl_index);
+}
+
 /**********************************************************************//**
 Adds system columns to a table object. */
 void