From b2654ba82651630dba0dd2012f45b77299a43548 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Thu, 1 Feb 2024 15:48:46 +0200 Subject: [PATCH] MDEV-32899 InnoDB is holding shared dict_sys.latch while waiting for FOREIGN KEY child table lock on DDL lock_table_children(): A new function to lock all child tables of a table. We will only hold dict_sys.latch while traversing dict_table_t::referenced_set. To prevent a race condition with std::set::erase() we will copy the pointers to the child tables to a local vector. Once we have acquired MDL and references to all child tables, we can safely release dict_sys.latch, wait for the locks, and finally release the references. dict_acquire_mdl_shared(): A new variant that takes mdl_context as a parameter. lock_table_for_trx(): Assert that we are not holding dict_sys.latch. ha_innobase::truncate(): When foreign_key_checks=ON, assert that no child tables exist (other than the current table). In any case, we will invoke lock_table_children() so that the child table metadata can be safely updated. (It is possible that a child table is being created concurrently with TRUNCATE TABLE.) ha_innobase::delete_table(): Before and after acquiring exclusive locks on the current table as well as all child tables, check that FOREIGN KEY constraints will not be violated. In this way, we can reject impossible DROP TABLE without having to wait for locks first. This fixes up commit 2ca112346438611ab7800b70bea6af1fd1169308 (MDEV-26217) and commit c3c53926c467c95386ae98d61ada87294bd61478 (MDEV-26554). --- mysql-test/suite/innodb/r/foreign_key.result | 21 +++- mysql-test/suite/innodb/t/foreign_key.test | 21 +++- storage/innobase/dict/dict0dict.cc | 82 ++++++++----- storage/innobase/handler/ha_innodb.cc | 120 +++++++++++-------- storage/innobase/handler/handler0alter.cc | 11 +- storage/innobase/include/dict0dict.h | 16 +++ storage/innobase/include/lock0lock.h | 7 ++ storage/innobase/lock/lock0lock.cc | 64 ++++++++++ 8 files changed, 248 insertions(+), 94 deletions(-) diff --git a/mysql-test/suite/innodb/r/foreign_key.result b/mysql-test/suite/innodb/r/foreign_key.result index 808e2270e27..80a0afb8d06 100644 --- a/mysql-test/suite/innodb/r/foreign_key.result +++ b/mysql-test/suite/innodb/r/foreign_key.result @@ -1035,9 +1035,22 @@ BEGIN; INSERT INTO child SET a=1; ERROR 23000: Cannot add or update a child row: a foreign key constraint fails (`test`.`child`, CONSTRAINT `child_ibfk_1` FOREIGN KEY (`a`) REFERENCES `parent` (`a`)) connection default; +TRUNCATE TABLE parent; +ERROR 42000: Cannot truncate a table referenced in a foreign key constraint (`test`.`child`, CONSTRAINT `child_ibfk_1` FOREIGN KEY (`a`) REFERENCES `test`.`parent` (`a`)) +DROP TABLE parent; +ERROR 23000: Cannot delete or update a parent row: a foreign key constraint fails +SET innodb_lock_wait_timeout=0; +RENAME TABLE parent TO transparent; +ERROR HY000: Lock wait timeout exceeded; try restarting transaction +ALTER TABLE parent FORCE, ALGORITHM=COPY; +ERROR HY000: Lock wait timeout exceeded; try restarting transaction +ALTER TABLE parent FORCE, ALGORITHM=INPLACE; +ERROR HY000: Lock wait timeout exceeded; try restarting transaction SET innodb_lock_wait_timeout=0, foreign_key_checks=0; TRUNCATE TABLE parent; ERROR HY000: Lock wait timeout exceeded; try restarting transaction +DROP TABLE parent; +ERROR HY000: Lock wait timeout exceeded; try restarting transaction ALTER TABLE parent FORCE, ALGORITHM=COPY; ERROR HY000: Lock wait timeout exceeded; try restarting transaction ALTER TABLE parent FORCE, ALGORITHM=INPLACE; @@ -1052,7 +1065,13 @@ TRUNCATE TABLE parent; ALTER TABLE parent FORCE, ALGORITHM=COPY; ALTER TABLE parent FORCE, ALGORITHM=INPLACE; ALTER TABLE parent ADD COLUMN b INT, ALGORITHM=INSTANT; -DROP TABLE child, parent; +SET foreign_key_checks=ON; +TRUNCATE TABLE parent; +ERROR 42000: Cannot truncate a table referenced in a foreign key constraint (`test`.`child`, CONSTRAINT `child_ibfk_1` FOREIGN KEY (`a`) REFERENCES `test`.`parent` (`a`)) +ALTER TABLE parent FORCE, ALGORITHM=COPY; +ALTER TABLE parent FORCE, ALGORITHM=INPLACE; +RENAME TABLE parent TO transparent; +DROP TABLE child, transparent; # # MDEV-26217 Failing assertion: list.count > 0 in ut_list_remove # or Assertion `lock->trx == this' failed in dberr_t trx_t::drop_table diff --git a/mysql-test/suite/innodb/t/foreign_key.test b/mysql-test/suite/innodb/t/foreign_key.test index 0db3a7ca377..e793e261abd 100644 --- a/mysql-test/suite/innodb/t/foreign_key.test +++ b/mysql-test/suite/innodb/t/foreign_key.test @@ -1077,10 +1077,23 @@ BEGIN; --error ER_NO_REFERENCED_ROW_2 INSERT INTO child SET a=1; connection default; +--error ER_TRUNCATE_ILLEGAL_FK +TRUNCATE TABLE parent; +--error ER_ROW_IS_REFERENCED_2 +DROP TABLE parent; +SET innodb_lock_wait_timeout=0; +--error ER_LOCK_WAIT_TIMEOUT +RENAME TABLE parent TO transparent; +--error ER_LOCK_WAIT_TIMEOUT +ALTER TABLE parent FORCE, ALGORITHM=COPY; +--error ER_LOCK_WAIT_TIMEOUT +ALTER TABLE parent FORCE, ALGORITHM=INPLACE; SET innodb_lock_wait_timeout=0, foreign_key_checks=0; --error ER_LOCK_WAIT_TIMEOUT TRUNCATE TABLE parent; --error ER_LOCK_WAIT_TIMEOUT +DROP TABLE parent; +--error ER_LOCK_WAIT_TIMEOUT ALTER TABLE parent FORCE, ALGORITHM=COPY; --error ER_LOCK_WAIT_TIMEOUT ALTER TABLE parent FORCE, ALGORITHM=INPLACE; @@ -1095,7 +1108,13 @@ TRUNCATE TABLE parent; ALTER TABLE parent FORCE, ALGORITHM=COPY; ALTER TABLE parent FORCE, ALGORITHM=INPLACE; ALTER TABLE parent ADD COLUMN b INT, ALGORITHM=INSTANT; -DROP TABLE child, parent; +SET foreign_key_checks=ON; +--error ER_TRUNCATE_ILLEGAL_FK +TRUNCATE TABLE parent; +ALTER TABLE parent FORCE, ALGORITHM=COPY; +ALTER TABLE parent FORCE, ALGORITHM=INPLACE; +RENAME TABLE parent TO transparent; +DROP TABLE child, transparent; --echo # --echo # MDEV-26217 Failing assertion: list.count > 0 in ut_list_remove diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc index 623c2c3dd7a..7f4f73d969b 100644 --- a/storage/innobase/dict/dict0dict.cc +++ b/storage/innobase/dict/dict0dict.cc @@ -657,47 +657,22 @@ dict_table_t::parse_name<>(char(&)[NAME_LEN + 1], char(&)[NAME_LEN + 1], /** Acquire MDL shared for the table name. @tparam trylock whether to use non-blocking operation @param[in,out] table table object -@param[in,out] thd background thread -@param[out] mdl mdl ticket +@param[in,out] mdl_context MDL context +@param[out] mdl MDL ticket @param[in] table_op operation to perform when opening @return table object after locking MDL shared @retval nullptr if the table is not readable, or if trylock && MDL blocked */ template +__attribute__((nonnull, warn_unused_result)) dict_table_t* dict_acquire_mdl_shared(dict_table_t *table, - THD *thd, - MDL_ticket **mdl, + MDL_context *mdl_context, MDL_ticket **mdl, dict_table_op_t table_op) { - if (!table || !mdl) - return table; - - MDL_context *mdl_context= static_cast(thd_mdl_context(thd)); - size_t db_len; - dict_table_t *not_found= nullptr; - - if (trylock) - { - dict_sys.freeze(SRW_LOCK_CALL); - db_len= dict_get_db_name_len(table->name.m_name); - dict_sys.unfreeze(); - } - else - { - ut_ad(dict_sys.frozen_not_locked()); - 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 */ - - if (!mdl_context) - return nullptr; - 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 tbl_len; + size_t db_len, tbl_len; bool unaccessible= false; if (!table->parse_name(db_buf, tbl_buf, &db_len, &tbl_len)) @@ -768,7 +743,6 @@ retry: if (!table || !table->is_accessible()) { - table= nullptr; return_without_mdl: if (trylock) dict_sys.unfreeze(); @@ -777,7 +751,7 @@ return_without_mdl: mdl_context->release_lock(*mdl); *mdl= nullptr; } - return not_found; + return nullptr; } size_t db1_len, tbl1_len; @@ -814,6 +788,50 @@ return_without_mdl: goto retry; } +template dict_table_t* dict_acquire_mdl_shared +(dict_table_t*,MDL_context*,MDL_ticket**,dict_table_op_t); + +/** Acquire MDL shared for the table name. +@tparam trylock whether to use non-blocking operation +@param[in,out] table table object +@param[in,out] thd background thread +@param[out] mdl mdl ticket +@param[in] table_op operation to perform when opening +@return table object after locking MDL shared +@retval nullptr if the table is not readable, or if trylock && MDL blocked */ +template +dict_table_t* +dict_acquire_mdl_shared(dict_table_t *table, + THD *thd, + MDL_ticket **mdl, + dict_table_op_t table_op) +{ + if (!table || !mdl) + return table; + + MDL_context *mdl_context= static_cast(thd_mdl_context(thd)); + size_t db_len; + + if (trylock) + { + dict_sys.freeze(SRW_LOCK_CALL); + db_len= dict_get_db_name_len(table->name.m_name); + dict_sys.unfreeze(); + } + else + { + ut_ad(dict_sys.frozen_not_locked()); + 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 */ + + return mdl_context + ? dict_acquire_mdl_shared(table, mdl_context, mdl, table_op) + : nullptr; +} + template dict_table_t* dict_acquire_mdl_shared (dict_table_t*,THD*,MDL_ticket**,dict_table_op_t); template dict_table_t* dict_acquire_mdl_shared diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 92f8a60ecf8..1cf6fa80ace 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -13395,6 +13395,49 @@ ha_innobase::discard_or_import_tablespace( DBUG_RETURN(0); } +/** Report a DROP TABLE failure due to a FOREIGN KEY constraint. +@param name table name +@param foreign constraint */ +ATTRIBUTE_COLD +static void delete_table_cannot_drop_foreign(const table_name_t &name, + const dict_foreign_t &foreign) +{ + mysql_mutex_lock(&dict_foreign_err_mutex); + rewind(dict_foreign_err_file); + ut_print_timestamp(dict_foreign_err_file); + fputs(" Cannot drop table ", dict_foreign_err_file); + ut_print_name(dict_foreign_err_file, nullptr, name.m_name); + fputs("\nbecause it is referenced by ", dict_foreign_err_file); + ut_print_name(dict_foreign_err_file, nullptr, foreign.foreign_table_name); + putc('\n', dict_foreign_err_file); + mysql_mutex_unlock(&dict_foreign_err_mutex); +} + +/** Check if DROP TABLE would fail due to a FOREIGN KEY constraint. +@param table table to be dropped +@param sqlcom thd_sql_command(current_thd) +@return whether child tables that refer to this table exist */ +static bool delete_table_check_foreigns(const dict_table_t &table, + enum_sql_command sqlcom) +{ + const bool drop_db{sqlcom == SQLCOM_DROP_DB}; + for (const auto foreign : table.referenced_set) + { + /* We should allow dropping a referenced table if creating + that referenced table has failed for some reason. For example + if referenced table is created but it column types that are + referenced do not match. */ + if (foreign->foreign_table == &table || + (drop_db && + dict_tables_have_same_db(table.name.m_name, + foreign->foreign_table_name_lookup))) + continue; + delete_table_cannot_drop_foreign(table.name, *foreign); + return true; + } + + return false; +} /** DROP TABLE (possibly as part of DROP DATABASE, CREATE/ALTER TABLE) @param name table name @@ -13410,6 +13453,7 @@ int ha_innobase::delete_table(const char *name) DBUG_EXECUTE_IF("test_normalize_table_name_low", test_normalize_table_name_low();); + const enum_sql_command sqlcom= enum_sql_command(thd_sql_command(thd)); trx_t *parent_trx= check_trx_exists(thd); dict_table_t *table; @@ -13446,6 +13490,13 @@ int ha_innobase::delete_table(const char *name) DBUG_RETURN(0); } + if (parent_trx->check_foreigns && + delete_table_check_foreigns(*table, sqlcom)) + { + dict_sys.unlock(); + DBUG_RETURN(HA_ERR_ROW_IS_REFERENCED); + } + table->acquire(); dict_sys.unlock(); @@ -13478,14 +13529,7 @@ int ha_innobase::delete_table(const char *name) /* FOREIGN KEY constraints cannot exist on partitioned tables. */; #endif else - { - dict_sys.freeze(SRW_LOCK_CALL); - for (const dict_foreign_t* f : table->referenced_set) - if (dict_table_t* child= f->foreign_table) - if ((err= lock_table_for_trx(child, trx, LOCK_X)) != DB_SUCCESS) - break; - dict_sys.unfreeze(); - } + err= lock_table_children(table, trx); } dict_table_t *table_stats= nullptr, *index_stats= nullptr; @@ -13495,7 +13539,6 @@ int ha_innobase::delete_table(const char *name) const bool fts= err == DB_SUCCESS && (table->flags2 & (DICT_TF2_FTS_HAS_DOC_ID | DICT_TF2_FTS)); - const enum_sql_command sqlcom= enum_sql_command(thd_sql_command(thd)); if (fts) { @@ -13653,36 +13696,16 @@ err_exit: DBUG_RETURN(convert_error_code_to_mysql(err, 0, NULL)); } - if (!table->no_rollback() && trx->check_foreigns) + if (!table->no_rollback()) { - const bool drop_db= sqlcom == SQLCOM_DROP_DB; - for (auto foreign : table->referenced_set) + if (trx->check_foreigns && delete_table_check_foreigns(*table, sqlcom)) { - /* We should allow dropping a referenced table if creating - that referenced table has failed for some reason. For example - if referenced table is created but it column types that are - referenced do not match. */ - if (foreign->foreign_table == table || - (drop_db && - dict_tables_have_same_db(table->name.m_name, - foreign->foreign_table_name_lookup))) - continue; - mysql_mutex_lock(&dict_foreign_err_mutex); - rewind(dict_foreign_err_file); - ut_print_timestamp(dict_foreign_err_file); - fputs(" Cannot drop table ", dict_foreign_err_file); - ut_print_name(dict_foreign_err_file, trx, table->name.m_name); - fputs("\nbecause it is referenced by ", dict_foreign_err_file); - ut_print_name(dict_foreign_err_file, trx, foreign->foreign_table_name); - putc('\n', dict_foreign_err_file); - mysql_mutex_unlock(&dict_foreign_err_mutex); err= DB_CANNOT_DROP_CONSTRAINT; goto err_exit; } - } - if (!table->no_rollback()) err= trx->drop_table_foreign(table->name); + } if (err == DB_SUCCESS && table_stats && index_stats) err= trx->drop_table_statistics(table->name); @@ -13801,6 +13824,19 @@ int ha_innobase::truncate() update_thd(); +#ifdef UNIV_DEBUG + if (!thd_test_options(m_user_thd, OPTION_NO_FOREIGN_KEY_CHECKS)) + { + /* fk_truncate_illegal_if_parent() should have failed in + Sql_cmd_truncate_table::handler_truncate() if foreign_key_checks=ON + and child tables exist. */ + dict_sys.freeze(SRW_LOCK_CALL); + for (const auto foreign : m_prebuilt->table->referenced_set) + ut_ad(foreign->foreign_table == m_prebuilt->table); + dict_sys.unfreeze(); + } +#endif + if (is_read_only()) DBUG_RETURN(HA_ERR_TABLE_READONLY); @@ -13883,14 +13919,7 @@ int ha_innobase::truncate() dict_table_t *table_stats = nullptr, *index_stats = nullptr; MDL_ticket *mdl_table = nullptr, *mdl_index = nullptr; - dberr_t error= DB_SUCCESS; - - dict_sys.freeze(SRW_LOCK_CALL); - for (const dict_foreign_t *f : ib_table->referenced_set) - if (dict_table_t *child= f->foreign_table) - if ((error= lock_table_for_trx(child, trx, LOCK_X)) != DB_SUCCESS) - break; - dict_sys.unfreeze(); + dberr_t error= lock_table_children(ib_table, trx); if (error == DB_SUCCESS) error= lock_table_for_trx(ib_table, trx, LOCK_X); @@ -14081,16 +14110,7 @@ ha_innobase::rename_table( /* There is no need to lock any FOREIGN KEY child tables. */ } else if (dict_table_t *table = dict_table_open_on_name( norm_from, false, DICT_ERR_IGNORE_FK_NOKEY)) { - dict_sys.freeze(SRW_LOCK_CALL); - for (const dict_foreign_t* f : table->referenced_set) { - if (dict_table_t* child = f->foreign_table) { - error = lock_table_for_trx(child, trx, LOCK_X); - if (error != DB_SUCCESS) { - break; - } - } - } - dict_sys.unfreeze(); + error = lock_table_children(table, trx); if (error == DB_SUCCESS) { error = lock_table_for_trx(table, trx, LOCK_X); } diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc index 3b892434a67..dc5c1f71925 100644 --- a/storage/innobase/handler/handler0alter.cc +++ b/storage/innobase/handler/handler0alter.cc @@ -11203,16 +11203,7 @@ ha_innobase::commit_inplace_alter_table( fts_optimize_remove_table(ctx->old_table); } - dict_sys.freeze(SRW_LOCK_CALL); - for (auto f : ctx->old_table->referenced_set) { - if (dict_table_t* child = f->foreign_table) { - error = lock_table_for_trx(child, trx, LOCK_X); - if (error != DB_SUCCESS) { - break; - } - } - } - dict_sys.unfreeze(); + error = lock_table_children(ctx->old_table, trx); if (ctx->new_table->fts) { ut_ad(!ctx->new_table->fts->add_wq); diff --git a/storage/innobase/include/dict0dict.h b/storage/innobase/include/dict0dict.h index 5b45f55f7c6..6f9a0efe02c 100644 --- a/storage/innobase/include/dict0dict.h +++ b/storage/innobase/include/dict0dict.h @@ -35,6 +35,7 @@ Created 1/8/1996 Heikki Tuuri #include #include +class MDL_context; class MDL_ticket; /** the first table or index ID for other than hard-coded system tables */ @@ -139,6 +140,21 @@ dict_acquire_mdl_shared(dict_table_t *table, MDL_ticket **mdl, dict_table_op_t table_op= DICT_TABLE_OP_NORMAL); +/** Acquire MDL shared for the table name. +@tparam trylock whether to use non-blocking operation +@param[in,out] table table object +@param[in,out] mdl_context MDL context +@param[out] mdl MDL ticket +@param[in] table_op operation to perform when opening +@return table object after locking MDL shared +@retval nullptr if the table is not readable, or if trylock && MDL blocked */ +template +__attribute__((nonnull, warn_unused_result)) +dict_table_t* +dict_acquire_mdl_shared(dict_table_t *table, + MDL_context *mdl_context, MDL_ticket **mdl, + dict_table_op_t table_op); + /** Look up a table by numeric identifier. @param[in] table_id table identifier @param[in] dict_locked data dictionary locked diff --git a/storage/innobase/include/lock0lock.h b/storage/innobase/include/lock0lock.h index e7b8e839c12..08b9f4bcb35 100644 --- a/storage/innobase/include/lock0lock.h +++ b/storage/innobase/include/lock0lock.h @@ -438,6 +438,13 @@ dberr_t lock_table_for_trx(dict_table_t *table, trx_t *trx, lock_mode mode, bool no_wait= false) MY_ATTRIBUTE((nonnull, warn_unused_result)); +/** Lock the child tables of a table. +@param table parent table +@param trx transaction +@return error code */ +dberr_t lock_table_children(dict_table_t *table, trx_t *trx) + MY_ATTRIBUTE((nonnull, warn_unused_result)); + /** Exclusively lock the data dictionary tables. @param trx dictionary transaction @return error code diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc index 6b1ca9d753f..907eaf58997 100644 --- a/storage/innobase/lock/lock0lock.cc +++ b/storage/innobase/lock/lock0lock.cc @@ -47,6 +47,7 @@ Created 5/7/1996 Heikki Tuuri #include "que0que.h" #include "scope.h" #include +#include #include @@ -3933,6 +3934,8 @@ static void lock_table_dequeue(lock_t *in_lock, bool owns_wait_mutex) dberr_t lock_table_for_trx(dict_table_t *table, trx_t *trx, lock_mode mode, bool no_wait) { + ut_ad(!dict_sys.frozen()); + mem_heap_t *heap= mem_heap_create(512); sel_node_t *node= sel_node_create(heap); que_thr_t *thr= pars_complete_graph_for_exec(node, trx, heap, nullptr); @@ -3969,6 +3972,67 @@ run_again: return err; } +/** Lock the child tables of a table. +@param table parent table +@param trx transaction +@return error code */ +dberr_t lock_table_children(dict_table_t *table, trx_t *trx) +{ + MDL_context *mdl_context= + static_cast(thd_mdl_context(trx->mysql_thd)); + ut_ad(mdl_context); + struct table_mdl{dict_table_t* table; MDL_ticket *mdl;}; + std::vector children; + children.emplace_back(table_mdl{table, nullptr}); + + dberr_t err= DB_SUCCESS; + dict_sys.freeze(SRW_LOCK_CALL); + + rescan: + for (auto f : table->referenced_set) + if (dict_table_t *child= f->foreign_table) + { + if (std::find_if(children.begin(), children.end(), + [&](const table_mdl &c){ return c.table == child; }) != + children.end()) + continue; /* We already acquired MDL on this child table. */ + MDL_ticket *mdl= nullptr; + child->acquire(); + child= dict_acquire_mdl_shared(child, mdl_context, &mdl, + DICT_TABLE_OP_NORMAL); + if (child) + { + if (!mdl) + child->release(); + children.emplace_back(table_mdl{child, mdl}); + goto rescan; + } + err= DB_LOCK_WAIT_TIMEOUT; + break; + } + dict_sys.unfreeze(); + + if (err == DB_SUCCESS) + for (const table_mdl &child : children) + if (child.mdl) + if ((err= lock_table_for_trx(child.table, trx, LOCK_X)) != DB_SUCCESS) + break; + + dict_sys.freeze(SRW_LOCK_CALL); + for (table_mdl &child : children) + { + if (child.mdl) + { + child.table->release(); + mdl_context->release_lock(child.mdl); + } + } + dict_sys.unfreeze(); + + return err; +} + + /** Exclusively lock the data dictionary tables. @param trx dictionary transaction @return error code