diff --git a/mysql-test/suite/innodb/r/dict_load_concurrent.result b/mysql-test/suite/innodb/r/dict_load_concurrent.result new file mode 100644 index 0000000000000..0e88c5ae8da85 --- /dev/null +++ b/mysql-test/suite/innodb/r/dict_load_concurrent.result @@ -0,0 +1,83 @@ +# +# MDEV-35154 dict_sys_t::load_table() is holding exclusive dict_sys.latch for unnecessarily long time +# +CREATE TABLE t1(a INT PRIMARY KEY) STATS_PERSISTENT=0 ENGINE=InnoDB; +CREATE TABLE t2(a INT PRIMARY KEY) STATS_PERSISTENT=0 ENGINE=InnoDB; +INSERT INTO t1 VALUES(1); +INSERT INTO t2 VALUES(2); +SET @save_evict= @@GLOBAL.innodb_evict_tables_on_commit_debug; +SET GLOBAL innodb_evict_tables_on_commit_debug=ON; +XA START 'x'; +INSERT INTO t1 VALUES(2); +INSERT INTO t2 VALUES(3); +XA END 'x'; +XA PREPARE 'x'; +connect con1,localhost,root,,; +FLUSH TABLES; +connection default; +XA COMMIT 'x'; +SET GLOBAL innodb_evict_tables_on_commit_debug=@save_evict; +connection con1; +SET DEBUG_SYNC = 'dict_load_table_one_no_latch SIGNAL loading WAIT_FOR go TIMEOUT 30'; +SELECT * FROM t1; +connection default; +SET DEBUG_SYNC = 'now WAIT_FOR loading TIMEOUT 30'; +SELECT * FROM t2; +a +2 +3 +connect con2,localhost,root,,; +SELECT * FROM t1; +connection default; +SET DEBUG_SYNC = 'now SIGNAL go'; +connection con1; +a +1 +2 +connection con2; +a +1 +2 +connection default; +SET DEBUG_SYNC = RESET; +DROP TABLE t1, t2; +CREATE TABLE parent(a INT PRIMARY KEY) STATS_PERSISTENT=0 ENGINE=InnoDB; +CREATE TABLE child(a INT PRIMARY KEY, +FOREIGN KEY(a) REFERENCES parent(a)) +STATS_PERSISTENT=0 ENGINE=InnoDB; +INSERT INTO parent VALUES(1); +INSERT INTO child VALUES(1); +SET GLOBAL innodb_evict_tables_on_commit_debug=ON; +XA START 'y'; +INSERT INTO parent VALUES(2); +INSERT INTO child VALUES(2); +XA END 'y'; +XA PREPARE 'y'; +connection con1; +FLUSH TABLES; +connection default; +XA COMMIT 'y'; +SET GLOBAL innodb_evict_tables_on_commit_debug=@save_evict; +connection con1; +SET DEBUG_SYNC = 'dict_load_table_one_no_latch SIGNAL child_loading WAIT_FOR child_go TIMEOUT 30'; +SELECT * FROM child; +connection con2; +SET DEBUG_SYNC = 'now WAIT_FOR child_loading TIMEOUT 30'; +SELECT * FROM parent; +connection default; +SET DEBUG_SYNC = 'now SIGNAL child_go'; +connection con1; +a +1 +2 +connection con2; +a +1 +2 +connection default; +DELETE FROM parent WHERE a=1; +ERROR 23000: Cannot delete or update a parent row: a foreign key constraint fails (`test`.`child`, CONSTRAINT `child_ibfk_1` FOREIGN KEY (`a`) REFERENCES `parent` (`a`)) +disconnect con1; +disconnect con2; +SET DEBUG_SYNC = RESET; +DROP TABLE child, parent; diff --git a/mysql-test/suite/innodb/t/dict_load_concurrent.test b/mysql-test/suite/innodb/t/dict_load_concurrent.test new file mode 100644 index 0000000000000..9cdaf120b77c3 --- /dev/null +++ b/mysql-test/suite/innodb/t/dict_load_concurrent.test @@ -0,0 +1,140 @@ +--source include/have_innodb.inc +--source include/have_debug.inc +--source include/have_debug_sync.inc +--source include/count_sessions.inc + +--echo # +--echo # MDEV-35154 dict_sys_t::load_table() is holding exclusive dict_sys.latch for unnecessarily long time +--echo # + +# Loading a table definition into the dictionary cache must not hold +# the exclusive dict_sys.latch during the I/O phase. A table whose +# definition is being loaded is published as an incomplete stub; +# other openers of the same table wait for the load to complete, +# while any other table can be opened (and loaded) concurrently. + +# STATS_PERSISTENT=0 keeps the background statistics thread from +# reloading the evicted table definitions behind our back. +CREATE TABLE t1(a INT PRIMARY KEY) STATS_PERSISTENT=0 ENGINE=InnoDB; +CREATE TABLE t2(a INT PRIMARY KEY) STATS_PERSISTENT=0 ENGINE=InnoDB; +INSERT INTO t1 VALUES(1); +INSERT INTO t2 VALUES(2); + +# Evict t1 and t2 from the dictionary cache: at XA COMMIT, the +# modified tables are evicted if no table handles are open, which +# FLUSH TABLES between XA PREPARE and XA COMMIT guarantees. The +# FLUSH must be issued from another connection, because a session +# with a prepared XA transaction refuses other statements. The +# prepared transaction's own table lock protects the tables from +# LRU eviction until the commit. +SET @save_evict= @@GLOBAL.innodb_evict_tables_on_commit_debug; +SET GLOBAL innodb_evict_tables_on_commit_debug=ON; +XA START 'x'; +INSERT INTO t1 VALUES(2); +INSERT INTO t2 VALUES(3); +XA END 'x'; +XA PREPARE 'x'; + +connect con1,localhost,root,,; +FLUSH TABLES; + +connection default; +XA COMMIT 'x'; +SET GLOBAL innodb_evict_tables_on_commit_debug=@save_evict; + +connection con1; +SET DEBUG_SYNC = 'dict_load_table_one_no_latch SIGNAL loading WAIT_FOR go TIMEOUT 30'; +send SELECT * FROM t1; + +connection default; +SET DEBUG_SYNC = 'now WAIT_FOR loading TIMEOUT 30'; + +# con1 is parked inside the load of t1 while holding no dict_sys.latch. +# Loading another table must not block. (Before this change, the +# following statement would hang until the load of t1 completed.) +SELECT * FROM t2; + +connect con2,localhost,root,,; +send SELECT * FROM t1; + +connection default; +# con2 must wait for the load of t1 (it observes the loading stub); +# con1 is parked at the DEBUG_SYNC point (state 'debug sync point: +# ...'), so exactly one session is waiting in 'Opening tables'. +let $wait_condition= + SELECT COUNT(*) = 1 FROM information_schema.processlist + WHERE state = 'Opening tables' AND info = 'SELECT * FROM t1'; +--source include/wait_condition.inc + +SET DEBUG_SYNC = 'now SIGNAL go'; + +connection con1; +reap; +connection con2; +reap; +connection default; +SET DEBUG_SYNC = RESET; +DROP TABLE t1, t2; + +# +# A table must not become visible while a table that is related to +# it by a FOREIGN KEY constraint is still being loaded by another +# thread: the constraint would be missing from referenced_set. +# +CREATE TABLE parent(a INT PRIMARY KEY) STATS_PERSISTENT=0 ENGINE=InnoDB; +CREATE TABLE child(a INT PRIMARY KEY, + FOREIGN KEY(a) REFERENCES parent(a)) +STATS_PERSISTENT=0 ENGINE=InnoDB; +INSERT INTO parent VALUES(1); +INSERT INTO child VALUES(1); + +SET GLOBAL innodb_evict_tables_on_commit_debug=ON; +XA START 'y'; +INSERT INTO parent VALUES(2); +INSERT INTO child VALUES(2); +XA END 'y'; +XA PREPARE 'y'; + +connection con1; +FLUSH TABLES; + +connection default; +XA COMMIT 'y'; +SET GLOBAL innodb_evict_tables_on_commit_debug=@save_evict; + +# Park inside the load of the child table. +connection con1; +SET DEBUG_SYNC = 'dict_load_table_one_no_latch SIGNAL child_loading WAIT_FOR child_go TIMEOUT 30'; +send SELECT * FROM child; + +connection con2; +SET DEBUG_SYNC = 'now WAIT_FOR child_loading TIMEOUT 30'; +# Loading the parent finds the constraint in SYS_FOREIGN, but the child +# is still an incomplete stub, so the constraint cannot be linked yet. +# The parent must not become visible before the child has been loaded. +send SELECT * FROM parent; + +connection default; +let $wait_condition= + SELECT COUNT(*) = 1 FROM information_schema.processlist + WHERE state = 'Opening tables' AND info = 'SELECT * FROM parent'; +--source include/wait_condition.inc + +SET DEBUG_SYNC = 'now SIGNAL child_go'; + +connection con1; +reap; +connection con2; +reap; + +connection default; +# The constraint was linked into parent->referenced_set before the +# parent became visible. +--error ER_ROW_IS_REFERENCED_2 +DELETE FROM parent WHERE a=1; + +disconnect con1; +disconnect con2; +SET DEBUG_SYNC = RESET; +DROP TABLE child, parent; +--source include/wait_until_count_sessions.inc diff --git a/storage/innobase/btr/btr0sea.cc b/storage/innobase/btr/btr0sea.cc index ee2fdde991e67..8681562e68463 100644 --- a/storage/innobase/btr/btr0sea.cc +++ b/storage/innobase/btr/btr0sea.cc @@ -245,6 +245,15 @@ ATTRIBUTE_COLD bool btr_search_disable() for (table = UT_LIST_GET_FIRST(dict_sys.table_non_LRU); table; table = UT_LIST_GET_NEXT(table_LRU, table)) { + if (table->loading()) { + /* The table definition is being loaded by another + thread without dict_sys.latch; walking its indexes + would race with that thread. The adaptive hash index + can only point to records of indexes whose metadata + is already cached. */ + continue; + } + btr_search_disable_ref_count(table); } diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc index 10eaf7a33408b..ead0bddaab2ed 100644 --- a/storage/innobase/dict/dict0dict.cc +++ b/storage/innobase/dict/dict0dict.cc @@ -597,7 +597,7 @@ dict_table_t *dict_sys_t::acquire_temporary_table(table_id_t id) const noexcept }); } -dict_table_t *dict_sys_t::find_table(table_id_t id) const noexcept +dict_table_t *dict_sys_t::find_table_any(table_id_t id) const noexcept { ut_ad(frozen()); return table_id_hash.cell_get(ut_fold_ull(id))-> @@ -609,7 +609,24 @@ dict_table_t *dict_sys_t::find_table(table_id_t id) const noexcept }); } -dict_table_t *dict_sys_t::find_table(const span &name) +dict_table_t *dict_sys_t::find_table(table_id_t id, bool excl) noexcept +{ + for (;;) + { + dict_table_t *t= find_table_any(id); + if (!t || !t->loading()) + return t; + /* Another thread is loading the table definition. Wait for it to + finish; on success t is valid to return directly, but if it + failed, retry from the top (the stub may have been freed, or a + fresh load of the same id may now be in progress). */ + ut_ad(!t->is_loader()); + if (wait_for_load(t, excl)) + return t; + } +} + +dict_table_t *dict_sys_t::find_table_any(const span &name) const noexcept { ut_ad(frozen()); @@ -621,6 +638,41 @@ dict_table_t *dict_sys_t::find_table(const span &name) }); } +dict_table_t *dict_sys_t::find_table(const span &name, bool excl) + noexcept +{ + for (;;) + { + dict_table_t *t= find_table_any(name); + if (!t || !t->loading()) + return t; + /* Another thread is loading the table definition. Wait for it to + finish; on success t is valid to return directly, but if it + failed, retry from the top (the stub may have been freed, or a + fresh load of the same name may now be in progress). */ + ut_ad(!t->is_loader()); + if (wait_for_load(t, excl)) + return t; + } +} + +dict_table_t *dict_sys_t::find_table_fk(const span &name) + const noexcept +{ + ut_ad(locked()); + dict_table_t *t= find_table_any(name); + /* A table whose definition is still being loaded (LOADING_DEF) is + concurrently modified by its loader without any latch; it must be + treated as if it were not cached. A table that is only waiting for + its FOREIGN KEY related tables to be loaded (LOADING_FK) has a + complete definition, and its foreign_set/referenced_set are only + modified under the exclusive latch, which we are holding; linking + a constraint into it is safe, and necessary so that two concurrent + dict_sys_t::load_table() invocations whose tables reference each + other will resolve the constraints between them. */ + return t && t->loading() == dict_table_t::LOADING_DEF ? nullptr : t; +} + /** Acquire MDL shared for the table name. @tparam trylock whether to use non-blocking operation @param[in,out] table table object @@ -689,7 +741,7 @@ 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); + table= dict_sys.find_table(table_id, false); if (table) { if (!table->is_accessible()) @@ -825,7 +877,7 @@ dict_table_t *dict_table_open_on_id(table_id_t table_id, bool dict_locked, if (!dict_locked) dict_sys.freeze(SRW_LOCK_CALL); - dict_table_t *table= dict_sys.find_table(table_id); + dict_table_t *table= dict_sys.find_table(table_id, dict_locked); if (table) { @@ -978,6 +1030,29 @@ void dict_sys_t::lock_wait(SRW_LOCK_ARGS(const char *file, unsigned line)) noexc latch.wr_lock(SRW_LOCK_ARGS(file, line)); } +bool dict_sys_t::wait_for_load(dict_table_t *table, bool excl) noexcept +{ + ut_ad(excl ? locked() : frozen_not_locked()); + ut_ad(!table->is_loader()); + if (!table->try_pin_for_wait()) + /* The load already failed; nothing to wait for. */ + return false; + /* Pin the table so it survives releasing dict_sys.latch: blocking + on lock_latch while still holding dict_sys.latch could deadlock + against a loader that needs dict_sys.latch to link FOREIGN KEY + constraints. */ + if (excl) unlock(); else unfreeze(); + table->lock_shared_lock(); + table->lock_shared_unlock(); + if (excl) lock(SRW_LOCK_CALL); else freeze(SRW_LOCK_CALL); + /* The loader released lock_latch only after settling into + NOT_LOADING or LOAD_FAILED; check while our pin still guarantees + table is valid, then release it (dict_sys.latch takes over). */ + const bool ready= !table->loading(); + table->release(); + return ready; +} + #ifdef UNIV_PFS_RWLOCK ATTRIBUTE_NOINLINE void dict_sys_t::unlock() noexcept { @@ -1031,7 +1106,7 @@ dict_table_open_on_name( if (!dict_locked) { dict_sys.freeze(SRW_LOCK_CALL); - table= dict_sys.find_table(name); + table= dict_sys.find_table(name, false); if (table) { ut_ad(table->cached); @@ -1144,7 +1219,10 @@ dict_table_add_system_columns( { ut_ad(table->n_def == table->n_cols - DATA_N_SYS_COLS); ut_ad(table->magic_n == DICT_TABLE_MAGIC_N); - ut_ad(!table->cached); + /* A table that is being loaded by dict_load_table_one() was + published in the cache as an incomplete stub before its columns + were loaded. */ + ut_ad(!table->cached || table->is_loader()); /* NOTE: the system columns MUST be added in the following order (so that they can be indexed by the numerical value of DATA_ROW_ID, @@ -1187,12 +1265,17 @@ inline void dict_sys_t::add(dict_table_t *table) noexcept table->row_id= 0; table->autoinc_mutex.init(); table->lock_mutex_init(); + if (table->loading()) + /* Take lock_latch before the stub is reachable via find_table_any(). */ + table->lock_mutex_lock(); const char *name= table->name.m_name; dict_table_t **prev= table_hash.cell_get(my_crc32c(0, name, strlen(name)))-> search(&dict_table_t::name_hash, [name](const dict_table_t *t) { if (!t) return true; - ut_ad(t->cached); + /* t may be a stub that another thread is loading; check the + atomic flag before the bit-field to avoid a torn read. */ + ut_ad(t->loading() || t->cached); ut_a(strcmp(t->name.m_name, name)); return false; }); @@ -1202,7 +1285,7 @@ inline void dict_sys_t::add(dict_table_t *table) noexcept search(&dict_table_t::id_hash, [table](const dict_table_t *t) { if (!t) return true; - ut_ad(t->cached); + ut_ad(t->loading() || t->cached); ut_a(t->id != table->id); return false; }); @@ -1221,6 +1304,11 @@ static bool dict_table_can_be_evicted(dict_table_t *table) { ut_ad(dict_sys.locked()); ut_a(table->can_be_evicted); + + if (table->loading()) { + return false; + } + ut_a(table->foreign_set.empty()); ut_a(table->referenced_set.empty()); @@ -1863,7 +1951,10 @@ void dict_sys_t::remove(dict_table_t* table, bool lru, bool keep) noexcept dict_index_t* index; ut_ad(dict_lru_validate()); - ut_a(table->get_ref_count() == 0); + /* A stub discarded after a failed load (dict_load_table_one_discard()) + still carries LOAD_FAILED in n_ref_count; only its reference count + bits must have drained to zero. */ + ut_a((table->get_ref_count() & ~dict_table_t::LOADING_MASK) == 0); ut_a(table->n_rec_locks == 0); ut_ad(find(table)); ut_ad(table->magic_n == DICT_TABLE_MAGIC_N); @@ -1986,7 +2077,7 @@ dict_index_add_to_cache( ulint n_ord; ulint i; - ut_ad(dict_sys.locked()); + ut_ad(dict_sys.locked() || index->table->is_loader()); ut_ad(index->n_def == index->n_fields); ut_ad(index->magic_n == DICT_INDEX_MAGIC_N); ut_ad(!dict_index_is_online_ddl(index)); @@ -2190,7 +2281,7 @@ dict_index_find_cols( const dict_table_t* table = index->table; ut_ad(table->magic_n == DICT_TABLE_MAGIC_N); - ut_ad(dict_sys.locked()); + ut_ad(dict_sys.locked() || table->is_loader()); for (ulint i = 0; i < index->n_fields; i++) { ulint j; @@ -2452,7 +2543,7 @@ dict_index_build_internal_clust( ut_ad(index->is_primary()); ut_ad(!index->has_virtual()); - ut_ad(dict_sys.locked()); + ut_ad(dict_sys.locked() || table->is_loader()); /* Create a new index object with certainly enough fields */ new_index = dict_mem_index_create(index->table, index->name, @@ -2605,7 +2696,7 @@ dict_index_build_internal_non_clust( ibool* indexed; ut_ad(!index->is_primary()); - ut_ad(dict_sys.locked()); + ut_ad(dict_sys.locked() || table->is_loader()); /* The clustered index should be the first in the list of indexes */ clust_index = UT_LIST_GET_FIRST(table->indexes); @@ -2693,7 +2784,7 @@ dict_index_build_internal_fts( dict_index_t* new_index; ut_ad(index->type & DICT_FTS); - ut_ad(dict_sys.locked()); + ut_ad(dict_sys.locked() || index->table->is_loader()); /* Create a new index */ new_index = dict_mem_index_create(index->table, index->name, @@ -2912,11 +3003,11 @@ dict_foreign_add_to_cache( ut_ad(dict_sys.locked()); - for_table = dict_sys.find_table( + for_table = dict_sys.find_table_fk( {foreign->foreign_table_name_lookup, strlen(foreign->foreign_table_name_lookup)}); - ref_table = dict_sys.find_table( + ref_table = dict_sys.find_table_fk( {foreign->referenced_table_name_lookup, strlen(foreign->referenced_table_name_lookup)}); ut_a(for_table || ref_table); @@ -4352,7 +4443,7 @@ dict_fs2utf8( @param id_hash dict_sys.table_id_hash or dict_sys.temp_id_hash */ static void hash_insert(dict_table_t *table, hash_table_t& id_hash) noexcept { - ut_ad(table->cached); + ut_ad(table->loading() || table->cached); dict_sys.table_hash.cell_get(my_crc32c(0, table->name.m_name, strlen(table->name.m_name)))-> append(*table, &dict_table_t::name_hash); diff --git a/storage/innobase/dict/dict0load.cc b/storage/innobase/dict/dict0load.cc index b5a1f55e3ec91..7d05e41d02217 100644 --- a/storage/innobase/dict/dict0load.cc +++ b/storage/innobase/dict/dict0load.cc @@ -57,15 +57,22 @@ key constraints are loaded into memory. @param[in] name Table name in the db/tablename format @param[in] ignore_err Error to be ignored when loading table and its index definition +@param[in] fk_heap Heap on which the names appended to + fk_tables are allocated @param[out] fk_tables Related table names that must also be loaded to ensure that all foreign key constraints are loaded. +@param[in] hold_latch Whether to hold the exclusive + dict_sys.latch for the whole load; see + dict_load_hold_latch() @return table, possibly with file_unreadable flag set @retval nullptr if the table does not exist */ static dict_table_t *dict_load_table_one(mtr_t &mtr, const span &name, dict_err_ignore_t ignore_err, - dict_names_t &fk_tables); + mem_heap_t *fk_heap, + dict_names_t &fk_tables, + bool hold_latch); /** Load an index definition from a SYS_INDEXES record to dict_index_t. @return error message @@ -1311,7 +1318,7 @@ static dberr_t dict_load_columns(mtr_t &mtr, btr_pcur_t pcur; ulint n_skipped = 0; - ut_ad(dict_sys.locked()); + ut_ad(dict_sys.locked() || table->is_loader()); mtr.start(); @@ -1441,7 +1448,7 @@ dict_load_virtual_col(mtr_t &mtr, dict_table_t *table, bool uncommitted, dict_index_t* sys_virtual_index; btr_pcur_t pcur; - ut_ad(dict_sys.locked()); + ut_ad(dict_sys.locked() || table->is_loader()); mtr.start(); @@ -1688,7 +1695,7 @@ static dberr_t dict_load_fields(mtr_t &mtr, dict_index_t *index, { btr_pcur_t pcur; - ut_ad(dict_sys.locked()); + ut_ad(dict_sys.locked() || index->table->is_loader()); auto sp = mtr.get_savepoint(); dict_index_t* sys_index = dict_sys.sys_fields->indexes.start; @@ -1943,7 +1950,7 @@ dberr_t dict_load_indexes(mtr_t *mtr, dict_table_t *table, bool uncommitted, btr_pcur_t pcur; byte table_id[8]; - ut_ad(dict_sys.locked()); + ut_ad(dict_sys.locked() || table->is_loader()); mtr->start(); @@ -2228,11 +2235,17 @@ void dict_get_and_save_data_dir_path(dict_table_t *table) if (strncmp(fil_path_to_mysql_datadir, filepath, strlen(fil_path_to_mysql_datadir))) { - table->lock_mutex_lock(); + /* A loading thread already holds lock_latch exclusively (see + dict_sys_t::add()) and is the only thread that can reach this + table, so it must not re-acquire it here. */ + const bool need_latch= !table->loading(); + if (need_latch) + table->lock_mutex_lock(); table->flags|= 1 << DICT_TF_POS_DATA_DIR & ((1U << DICT_TF_BITS) - 1); table->data_dir_path= mem_heap_strdup(table->heap, filepath); os_file_make_data_dir_path(table->data_dir_path); - table->lock_mutex_unlock(); + if (need_latch) + table->lock_mutex_unlock(); } } } @@ -2303,6 +2316,61 @@ dict_load_tablespace( ut_free(filepath); } +/** Determine whether a table must be loaded without ever releasing +the exclusive dict_sys.latch, because it may be referenced by InnoDB +internal SQL. The non-reentrant internal SQL parser is serialized by +the exclusive dict_sys.latch, which the callers of que_eval_sql() hold +across parsing and execution; pars_retrieve_table_def() opens the +referenced tables in the middle of parsing. Releasing the latch there +(to publish a loading stub, or to wait for one in +dict_sys_t::wait_for_load()) would let another thread corrupt the +parser state. Because every load of such a table holds the latch throughout, +no loading stub can ever exist for it, and a parse-time lookup can +never encounter one. +@param name table name ("db/table" or an InnoDB system table name) +@return whether the table must be loaded while holding the latch */ +static bool dict_load_hold_latch(const span &name) +{ + const void* p = memchr(name.data(), '/', name.size()); + + if (!p) { + /* InnoDB system tables, such as SYS_FOREIGN, have no + database component in their names. */ + return true; + } + + const char* n = static_cast(p) + 1; + + if (name.end() > n + 4 + && n[0] == 'F' && n[1] == 'T' + && n[2] == 'S' && n[3] == '_') { + /* FULLTEXT INDEX auxiliary tables */ + return true; + } + + /* The persistent statistics tables */ + return (name.size() == sizeof TABLE_STATS_NAME - 1 + && !memcmp(name.data(), TABLE_STATS_NAME, name.size())) + || (name.size() == sizeof INDEX_STATS_NAME - 1 + && !memcmp(name.data(), INDEX_STATS_NAME, name.size())); +} + +/** Discard a table stub whose load failed (or is being retried after +DB_SUCCESS_LOCKED_REC), unlinking it from dict_sys and freeing it. +@param table a stub published by this thread via add_to_cache(), +still holding lock_latch in exclusive mode */ +static void dict_load_table_one_discard(dict_table_t *table) noexcept +{ + table->mark_load_failed(); + /* Wait for any wait_for_load() caller that had already pinned the + table to notice and release its pin; no new pin is possible once + LOAD_FAILED is set, so this can only reach zero, never rise. */ + while (!table->load_failed_refs_cleared()) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + dict_sys.remove(table); +} + /** Loads a table definition and also all its index definitions. Loads those foreign key constraints whose referenced table is already in @@ -2315,15 +2383,22 @@ key constraints are loaded into memory. @param[in] name Table name in the db/tablename format @param[in] ignore_err Error to be ignored when loading table and its index definition +@param[in] fk_heap Heap on which the names appended to + fk_tables are allocated @param[out] fk_tables Related table names that must also be loaded to ensure that all foreign key constraints are loaded. +@param[in] hold_latch Whether to hold the exclusive + dict_sys.latch for the whole load; see + dict_load_hold_latch() @return table, possibly with file_unreadable flag set @retval nullptr if the table does not exist */ static dict_table_t *dict_load_table_one(mtr_t &mtr, const span &name, dict_err_ignore_t ignore_err, - dict_names_t &fk_tables) + mem_heap_t *fk_heap, + dict_names_t &fk_tables, + bool hold_latch) { btr_pcur_t pcur; @@ -2332,6 +2407,7 @@ static dict_table_t *dict_load_table_one(mtr_t &mtr, ("table: %.*s", int(name.size()), name.data())); ut_ad(dict_sys.locked()); + ut_ad(!dict_load_hold_latch(name) || hold_latch); dict_index_t *sys_index = dict_sys.sys_tables->indexes.start; ut_ad(!dict_sys.sys_tables->not_redundant()); @@ -2358,7 +2434,20 @@ static dict_table_t *dict_load_table_one(mtr_t &mtr, pcur.btr_cur.page_cur.index = sys_index; bool uncommitted = false; + dict_table_t* table = nullptr; reload: + /* Phase 1 (exclusive dict_sys.latch): locate the SYS_TABLES + record, create the table object, and make it visible in the + cache as a non-evictable, incomplete stub (dict_table_t::loading()), + so that the latch can be released during the I/O below. */ + if (table) { + /* We are retrying after DB_SUCCESS_LOCKED_REC. Discard + the previously published stub; a new table object will + be created. */ + dict_load_table_one_discard(table); + table = nullptr; + } + mtr.start(); dberr_t err = btr_pcur_open_on_user_rec(&tuple, BTR_SEARCH_LEAF, &pcur, &mtr); @@ -2378,7 +2467,6 @@ static dict_table_t *dict_load_table_one(mtr_t &mtr, goto err_exit; } - dict_table_t* table; if (const char* err_msg = dict_load_table_low(&mtr, uncommitted, rec, &table)) { if (err_msg != dict_load_table_flags) { @@ -2398,6 +2486,29 @@ static dict_table_t *dict_load_table_one(mtr_t &mtr, mtr.commit(); + /* Publish the incomplete table object. Any other thread that + looks it up will observe dict_table_t::loading() and either treat + the table as not cached (dict_sys_t::find_table()) or wait for + the load to complete (dict_sys_t::load_table()). add_to_cache() + takes lock_latch in exclusive mode on our behalf before the stub + becomes reachable. The stub is inserted into table_non_LRU + (can_be_evicted=false), so that dict_sys_t::evict_table_LRU() + cannot free it. */ + ut_ad(!table->can_be_evicted); + table->start_loading(); + ut_d(table->load_thread = pthread_self()); + table->add_to_cache(); + + if (!hold_latch) { + dict_sys.unlock(); + } + + /* Phase 2 (no dict_sys.latch): open the tablespace and load the + columns and indexes. Only this thread may access the table object; + the SYS_ tables and their indexes are non-evictable, and any DDL + that would modify this table's SYS_ records must first look up the + table in the cache, observe the stub, and wait for it. */ + mem_heap_t* heap = mem_heap_create(32000); dict_load_tablespace(table, ignore_err); @@ -2406,8 +2517,10 @@ static dict_table_t *dict_load_table_one(mtr_t &mtr, case DB_SUCCESS_LOCKED_REC: ut_ad(!uncommitted); uncommitted = true; - dict_mem_table_free(table); mem_heap_free(heap); + if (!hold_latch) { + dict_sys.lock(SRW_LOCK_CALL); + } goto reload; case DB_SUCCESS: if (!dict_load_virtual(mtr, table, uncommitted)) { @@ -2415,16 +2528,16 @@ static dict_table_t *dict_load_table_one(mtr_t &mtr, } /* fall through */ default: - dict_mem_table_free(table); mem_heap_free(heap); + if (!hold_latch) { + dict_sys.lock(SRW_LOCK_CALL); + } + dict_load_table_one_discard(table); DBUG_RETURN(nullptr); } dict_table_add_system_columns(table, heap); - table->can_be_evicted = true; - table->add_to_cache(); - mem_heap_empty(heap); ut_ad(dict_tf2_is_valid(table->flags, table->flags2)); @@ -2443,19 +2556,16 @@ static dict_table_t *dict_load_table_one(mtr_t &mtr, err = dict_load_indexes(&mtr, table, uncommitted, heap, index_load_err); + bool evict_stub = false; + if (err == DB_TABLE_CORRUPT) { /* Refuse to load the table if the table has a corrupted cluster index */ ut_ad(index_load_err != DICT_ERR_IGNORE_DROP); ib::error() << "Refusing to load corrupted table " << table->name; -evict: - dict_sys.remove(table); - mem_heap_free(heap); - DBUG_RETURN(nullptr); - } - - if (err != DB_SUCCESS || !table->is_readable()) { + evict_stub = true; + } else if (err != DB_SUCCESS || !table->is_readable()) { } else if (dict_index_t* pk = dict_table_get_first_index(table)) { ut_ad(pk->is_primary()); if (pk->is_corrupted() @@ -2494,10 +2604,38 @@ static dict_table_t *dict_load_table_one(mtr_t &mtr, ut_ad(ignore_err & DICT_ERR_IGNORE_INDEX); if (ignore_err != DICT_ERR_IGNORE_DROP) { err = DB_CORRUPTION; - goto evict; + evict_stub = true; } } + if (!hold_latch) { + /* When the definition is loaded by a connection thread, + allow tests to run other statements while no + dict_sys.latch is held. */ + DEBUG_SYNC_C("dict_load_table_one_no_latch"); + + /* Phase 3 (exclusive dict_sys.latch): complete the + definition and load the foreign key constraints. */ + dict_sys.lock(SRW_LOCK_CALL); + } + + dict_sys.allow_eviction(table); + /* The definition is complete, but the table stays hidden from + dict_sys_t::find_table() until our caller dict_sys_t::load_table() + has also loaded the tables that are related to this table by + FOREIGN KEY constraints, and makes all of them visible atomically. + From this point on, constraints may be linked into this table + via dict_sys_t::find_table_fk(): the foreign_set/referenced_set + are only modified while holding the exclusive latch. */ + table->advance_to_loading_fk(); + + if (evict_stub) { +evict: + mem_heap_free(heap); + dict_load_table_one_discard(table); + DBUG_RETURN(nullptr); + } + /* We will load the foreign key information only if all indexes were loaded. */ if (!table->is_readable()) { @@ -2505,7 +2643,8 @@ static dict_table_t *dict_load_table_one(mtr_t &mtr, } else if (err == DB_SUCCESS) { auto i = fk_tables.size(); err = dict_load_foreigns(mtr, table->name.m_name, nullptr, - 0, true, ignore_err, fk_tables); + 0, true, ignore_err, fk_heap, + fk_tables); if (err != DB_SUCCESS) { fk_tables.erase(fk_tables.begin() + i, fk_tables.end()); @@ -2552,20 +2691,83 @@ static dict_table_t *dict_load_table_one(mtr_t &mtr, dict_table_t *dict_sys_t::load_table(const span &name, dict_err_ignore_t ignore) noexcept { - if (dict_table_t *table= find_table(name)) - return table; + for (;;) + { + dict_table_t *table= find_table_any(name); + if (!table) + break; + if (!table->loading()) + return table; + /* Another thread is loading the table definition. Wait for it + to complete or fail. wait_for_load() drops and reacquires + dict_sys.latch around the actual wait, but validates table + before releasing its own temporary pin on it, handing off + safety to dict_sys.latch; we hold that latch continuously from + here on, so on success table is still valid to return directly. */ + ut_ad(!table->is_loader()); + if (wait_for_load(table, true)) + return table; + } + + mem_heap_t *fk_heap= mem_heap_create(1000); dict_names_t fk_list; THD* const thd{current_thd}; mtr_t mtr{thd ? thd_to_trx(thd) : nullptr}; - dict_table_t *table= dict_load_table_one(mtr, name, ignore, fk_list); + /* The tables loaded by this invocation. They remain hidden from + other threads (dict_table_t::LOADING_FK) until all of them have + been loaded, so that no thread can operate on a table whose + foreign key constraints are not fully linked yet. Being hidden, + they cannot be referenced, dropped or evicted by any other thread + (dict_table_can_be_evicted() also skips them), so the pointers + remain valid across the latch releases below. */ + std::vector> loaded; + /* Tables that may be referenced by InnoDB internal SQL must be + loaded without ever releasing the exclusive latch, because this may + be a parse-time table lookup from pars_retrieve_table_def(), and the + non-reentrant internal SQL parser is serialized by the latch. Any + tables loaded for their FOREIGN KEY constraints inherit this, so + that no latch release can occur in the middle of parsing. */ + const bool hold_latch= dict_load_hold_latch(name); + dict_table_t *table= dict_load_table_one(mtr, name, ignore, fk_heap, fk_list, + hold_latch); + if (table) + loaded.emplace_back(table); while (!fk_list.empty()) { const char *f= fk_list.front(); - const span name{f, strlen(f)}; - if (!find_table(name)) - dict_load_table_one(mtr, name, ignore, fk_list); + const span fk_name{f, strlen(f)}; + if (dict_table_t *t= find_table_any(fk_name)) + { + if (t->loading() == dict_table_t::LOADING_DEF && !hold_latch) + { + /* Another thread is still loading the definition of this + table, so its dict_load_foreigns() has not been invoked yet + and the constraints between it and the tables that we loaded + may still be missing. Wait for it, so that we will not + publish a table whose referenced_set is incomplete. On + success its dict_load_foreigns() has since run and linked + whatever it could, same as if we had found it already loaded; + on failure, retry this name from scratch. */ + ut_ad(!t->is_loader()); + if (!wait_for_load(t, true)) + continue; + } + } + else if (dict_table_t *fk_table= + dict_load_table_one(mtr, fk_name, ignore, fk_heap, fk_list, + hold_latch || dict_load_hold_latch(fk_name))) + loaded.emplace_back(fk_table); fk_list.pop_front(); } + mem_heap_free(fk_heap); + + /* All tables related by foreign key constraints have been loaded + and linked; make them visible to other threads, and wake up any + dict_sys_t::wait_for_load() callers blocked on each one's + lock_latch. They will block on the exclusive dict_sys.latch until + our caller releases it, and then see the load as successful. */ + for (dict_table_t *t : loaded) + t->load_finish(); return table; } @@ -2588,10 +2790,6 @@ dict_load_table_on_id( ut_ad(dict_sys.locked()); - /* NOTE that the operation of this function is protected by - dict_sys.latch, and therefore no deadlocks can occur - with other dictionary operations. */ - mtr.start(); /*---------------------------------------------------*/ /* Get the secondary index based on ID for table SYS_TABLES */ @@ -2628,16 +2826,46 @@ dict_load_table_on_id( /* Check if the table id in record is the one searched for */ if (table_id == mach_read_from_8(field)) { + char name_buf[MAX_FULL_NAME_LEN + 1]; field = rec_get_nth_field_old(rec, DICT_FLD__SYS_TABLE_IDS__NAME, &len); - table = dict_sys.load_table( - {reinterpret_cast(field), - len}, ignore_err); + if (UNIV_UNLIKELY(len > sizeof name_buf)) { + /* corrupted record */ + goto func_exit; + } + /* Copy the name and release the page latch before + calling load_table(): it releases the exclusive + dict_sys.latch during the load, and it may block + waiting for a concurrent load of the same table. + Waiting while holding a latched SYS_TABLES page + could deadlock with a DDL thread that holds the + exclusive dict_sys.latch and is waiting for a + latch on this page. */ + memcpy(name_buf, field, len); + ut_d(const auto deleted = rec_get_deleted_flag(rec, + 0)); + btr_pcur_store_position(&pcur, &mtr); + mtr.commit(); + + table = dict_sys.load_table({name_buf, len}, + ignore_err); if (table && table->id != table_id) { - ut_ad(rec_get_deleted_flag(rec, 0)); + /* The record was delete-marked, and the + name has been reused by another table. */ + ut_ad(deleted); table = nullptr; } - if (!table) { + if (table) { + ut_free(pcur.old_rec_buf); + return table; + } + + /* The SYS_TABLE_IDS record may have been stale. + Keep scanning for further records with a + matching table id. */ + mtr.start(); + if (pcur.restore_position(BTR_SEARCH_LEAF, &mtr) + != btr_pcur_t::CORRUPTED) { while (btr_pcur_move_to_next(&pcur, &mtr)) { rec = btr_pcur_get_rec(&pcur); @@ -2649,7 +2877,9 @@ dict_load_table_on_id( } } +func_exit: mtr.commit(); + ut_free(pcur.old_rec_buf); return table; } @@ -2838,6 +3068,9 @@ dict_load_foreign( /*!< in: foreign constraint id */ dict_err_ignore_t ignore_err, /*!< in: error to be ignored */ + mem_heap_t* fk_heap, + /*!< in: heap for the names appended + to fk_tables */ dict_names_t& fk_tables) /*!< out: the foreign key constraint is added to the dictionary cache only if the referenced @@ -2986,10 +3219,10 @@ dict_load_foreign( goto load_error; } - ref_table = dict_sys.find_table( + ref_table = dict_sys.find_table_fk( {foreign->referenced_table_name_lookup, strlen(foreign->referenced_table_name_lookup)}); - for_table = dict_sys.find_table( + for_table = dict_sys.find_table_fk( {foreign->foreign_table_name_lookup, strlen(foreign->foreign_table_name_lookup)}); @@ -2997,11 +3230,15 @@ dict_load_foreign( /* To avoid recursively loading the tables related through the foreign key constraints, the child table name is saved here. The child table will be loaded later, along with its - foreign key constraint. */ + foreign key constraint. The name is copied to fk_heap, + which the caller keeps valid until fk_tables is drained; + ref_table->heap would not do, because ref_table may be + evicted while dict_sys.load_table() releases the exclusive + dict_sys.latch between the drain iterations. */ ut_a(ref_table != NULL); fk_tables.push_back( - mem_heap_strdupl(ref_table->heap, + mem_heap_strdupl(fk_heap, foreign->foreign_table_name_lookup, foreign_table_name_len)); load_error: @@ -3047,6 +3284,9 @@ dict_load_foreigns( bool check_charsets, /*!< in: whether to check charset compatibility */ dict_err_ignore_t ignore_err, /*!< in: error to be ignored */ + mem_heap_t* fk_heap, /*!< in: heap on which the + names appended to fk_tables + are allocated */ dict_names_t& fk_tables) /*!< out: stack of table names which must be loaded @@ -3156,7 +3396,8 @@ dict_load_foreigns( err = len < sizeof fk_id ? dict_load_foreign(mtr, table_name, false, col_names, trx_id, check_recursive, check_charsets, - {fk_id, len}, ignore_err, fk_tables) + {fk_id, len}, ignore_err, fk_heap, + fk_tables) : DB_CORRUPTION; switch (err) { diff --git a/storage/innobase/fts/fts0fts.cc b/storage/innobase/fts/fts0fts.cc index e48e24e691728..0e95447946199 100644 --- a/storage/innobase/fts/fts0fts.cc +++ b/storage/innobase/fts/fts0fts.cc @@ -1524,6 +1524,7 @@ static dberr_t fts_lock_table(trx_t *trx, const char *table_name) DICT_ERR_IGNORE_TABLESPACE)) { dberr_t err= lock_table_for_trx(table, trx, LOCK_X); + ut_ad(!table->loading()); /* Wait for purge threads to stop using the table. */ for (uint n= 15; table->get_ref_count() > 1; ) { @@ -1591,6 +1592,7 @@ static void fts_table_no_ref_count(const char *table_name) if (!table) return; + ut_ad(!table->loading()); while (table->get_ref_count() > 1) { dict_sys.unlock(); diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 0f4a5cab1a5a8..e364c23be05bb 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -1254,7 +1254,10 @@ static void innodb_drop_database(handlerton*, char *path) next= table->id_hash; if (strncmp(table->name.m_name, namebuf, len + 1)) continue; - const auto n_handles= table->get_ref_count(); + /* This walks the hash table directly, so table may still be + loading(); its LOADING_MASK bits are not open handles. */ + const auto n_handles= + table->get_ref_count() & ~dict_table_t::LOADING_MASK; const bool locks= !n_handles && lock_table_has_locks(table); if (n_handles || locks) { @@ -12618,6 +12621,17 @@ create_table_info_t::create_foreign_keys() { dict_foreign_set local_fk_set; dict_foreign_set_free local_fk_set_free(local_fk_set); + /* Release the temporary reference taken below on each + foreign->referenced_table left in local_fk_set on failure; the + success path releases them itself and clears the set first, so + this is then a no-op. Declared after local_fk_set_free so it runs + first (LIFO), before dict_foreign_free() invalidates `foreign`. */ + auto unpin_referenced_tables = make_scope_exit([&local_fk_set]() { + for (const dict_foreign_t *foreign : local_fk_set) + if (dict_table_t *rt = foreign->referenced_table) { + rt->release(); + } + }); dberr_t error; ulint number = 1; static const unsigned MAX_COLS_PER_FK = 500; @@ -12685,7 +12699,7 @@ create_table_info_t::create_foreign_keys() ut_ad(alter_info); List_iterator_fast key_it(alter_info->key_list); - dict_table_t* table = dict_sys.find_table({name.str, name.length}); + dict_table_t* table = dict_sys.find_table({name.str, name.length}, true); if (!table) { ib_foreign_warn(m_trx, DB_CANNOT_ADD_CONSTRAINT, create_name, "%s table %s foreign key constraint" @@ -12839,6 +12853,16 @@ create_table_info_t::create_foreign_keys() foreign->referenced_table_name = dict_table_lookup( d, t, &foreign->referenced_table, foreign->heap); + if (foreign->referenced_table) { + /* Pin it so dict_sys.load_table() for a later + constraint in this loop cannot evict it while + referenced_set does not yet reflect this constraint. + unpin_referenced_tables releases this on failure; + made permanent via dict_sys.prevent_eviction() only + once the constraint is actually committed below. */ + foreign->referenced_table->acquire(); + } + if (!foreign->referenced_table && m_trx->check_foreigns) { char buf[MAX_TABLE_NAME_LEN + 1] = ""; char* bufend; @@ -13041,6 +13065,14 @@ create_table_info_t::create_foreign_keys() local_fk_set.end()); std::for_each(local_fk_set.begin(), local_fk_set.end(), dict_foreign_add_to_referenced_table()); + for (const dict_foreign_t *foreign : local_fk_set) { + if (dict_table_t *rt = foreign->referenced_table) { + /* Committed: make it permanently + non-evictable, replacing the temporary pin. */ + dict_sys.prevent_eviction(rt); + rt->release(); + } + } local_fk_set.clear(); dict_mem_table_fill_foreign_vcol_set(table); @@ -13170,6 +13202,13 @@ int create_table_info_t::create_table(bool create_fk) } create_fk&= !m_creating_stub; + + /* Pin the newly created table: dict_sys.load_table() inside + create_foreign_keys() and in the loop below may temporarily + release the exclusive dict_sys.latch, during which + dict_sys.evict_table_LRU() could evict an unreferenced table. */ + m_table->acquire(); + dberr_t err = create_fk ? create_foreign_keys() : DB_SUCCESS; if (err == DB_SUCCESS) { @@ -13179,17 +13218,24 @@ int create_table_info_t::create_table(bool create_fk) /* Check that also referencing constraints are ok */ dict_names_t fk_tables; mtr_t mtr{m_trx}; + /* The names in fk_tables must survive the temporary + release of the exclusive dict_sys.latch inside + dict_sys.load_table() in the loop below. */ + mem_heap_t* fk_heap = mem_heap_create(1000); err = dict_load_foreigns(mtr, m_table_name, nullptr, m_trx->id, true, - ignore_err, fk_tables); + ignore_err, fk_heap, fk_tables); while (err == DB_SUCCESS && !fk_tables.empty()) { dict_sys.load_table( {fk_tables.front(), strlen(fk_tables.front())}, ignore_err); fk_tables.pop_front(); } + mem_heap_free(fk_heap); } + m_table->release(); + switch (err) { case DB_PARENT_NO_INDEX: push_warning_printf( @@ -13972,6 +14018,7 @@ int ha_innobase::delete_table(const char *name) dict_sys.lock(SRW_LOCK_CALL); + ut_ad(!table->loading()); if (!table->release() && err == DB_SUCCESS) { /* Wait for purge threads to stop using the table. */ @@ -14247,6 +14294,7 @@ int ha_innobase::truncate() if (error == DB_SUCCESS) error= lock_table_for_trx(ib_table, trx, LOCK_X); + ut_ad(!ib_table->loading()); const bool fts= error == DB_SUCCESS && ib_table->flags2 & (DICT_TF2_FTS_HAS_DOC_ID | DICT_TF2_FTS); const bool pause_purge= error == DB_SUCCESS && ib_table->get_ref_count() > 1; @@ -14333,13 +14381,17 @@ int ha_innobase::truncate() m_prebuilt->table->def_trx_id= def_trx_id; } dict_names_t fk_tables; + /* The names in fk_tables must survive the temporary release of the + exclusive dict_sys.latch inside dict_sys.load_table() below. */ + mem_heap_t *fk_heap= mem_heap_create(1000); { mtr_t mtr{trx}; dict_load_foreigns(mtr, m_prebuilt->table->name.m_name, nullptr, 1, true, - DICT_ERR_IGNORE_FK_NOKEY, fk_tables); + DICT_ERR_IGNORE_FK_NOKEY, fk_heap, fk_tables); } for (const char *f : fk_tables) dict_sys.load_table({f, strlen(f)}); + mem_heap_free(fk_heap); } if (fts) @@ -14461,6 +14513,7 @@ ha_innobase::rename_table( t = dict_table_open_on_name( norm_from, false, DICT_ERR_IGNORE_FK_NOKEY); if (t) { + ut_ad(!t->loading()); error = lock_table_children(t, trx); if (error == DB_SUCCESS) { error = lock_table_for_trx(t, trx, LOCK_X); diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc index 2a8caedb1c8b0..0532b0a0b7187 100644 --- a/storage/innobase/handler/handler0alter.cc +++ b/storage/innobase/handler/handler0alter.cc @@ -6647,6 +6647,7 @@ prepare_inplace_alter_table_dict( const bool have_fts = user_table->flags2 & (DICT_TF2_FTS_HAS_DOC_ID | DICT_TF2_FTS); + ut_ad(!user_table->loading()); const bool pause_purge = have_fts || user_table->get_ref_count() > 1; /* Acquire a lock on the table before creating any indexes. */ bool table_lock_failed = false; @@ -7206,7 +7207,7 @@ prepare_inplace_alter_table_dict( if (dict_sys.find_table( {ctx->new_table->name.m_name, - strlen(ctx->new_table->name.m_name)})) { + strlen(ctx->new_table->name.m_name)}, true)) { my_error(ER_TABLE_EXISTS_ERROR, MYF(0), ctx->new_table->name.m_name); goto new_clustered_failed; @@ -10181,11 +10182,15 @@ innobase_update_foreign_cache( dictionary cache (work around the lack of WL#6049). */ dict_names_t fk_tables; mtr_t mtr{ctx->trx}; + /* The names in fk_tables must survive the temporary release of + the exclusive dict_sys.latch inside dict_sys.load_table() in the + loop below. */ + mem_heap_t* fk_heap = mem_heap_create(1000); err = dict_load_foreigns(mtr, user_table->name.m_name, ctx->col_names, 1, true, DICT_ERR_IGNORE_FK_NOKEY, - fk_tables); + fk_heap, fk_tables); if (err == DB_CANNOT_ADD_CONSTRAINT) { fk_tables.clear(); @@ -10196,7 +10201,7 @@ innobase_update_foreign_cache( err = dict_load_foreigns(mtr, user_table->name.m_name, ctx->col_names, 1, false, DICT_ERR_IGNORE_NONE, - fk_tables); + fk_heap, fk_tables); /* The load with "charset_check" off is successful, warn the user that the foreign key has loaded with mis-matched @@ -10229,6 +10234,8 @@ innobase_update_foreign_cache( fk_tables.pop_front(); } + mem_heap_free(fk_heap); + DBUG_RETURN(err); } diff --git a/storage/innobase/handler/i_s.cc b/storage/innobase/handler/i_s.cc index fe81dbc65eae0..007f25748d690 100644 --- a/storage/innobase/handler/i_s.cc +++ b/storage/innobase/handler/i_s.cc @@ -4698,6 +4698,8 @@ i_s_dict_fill_sys_tablestats(THD* thd, dict_table_t *table, Field **fields= table_to_fill->field; + ut_ad(!table->loading()); + { table->stats_shared_lock(); auto _ = make_scope_exit([table]() { diff --git a/storage/innobase/include/dict0dict.h b/storage/innobase/include/dict0dict.h index 091f2e87a0f29..1874d909bbb3a 100644 --- a/storage/innobase/include/dict0dict.h +++ b/storage/innobase/include/dict0dict.h @@ -1366,11 +1366,23 @@ class dict_sys_t (should only happen during the rollback of CREATE...SELECT) */ dict_table_t *acquire_temporary_table(table_id_t id) const noexcept; - /** Look up a persistent table. + /** Look up a persistent table, including one whose definition is + still being loaded (dict_table_t::loading()). Of such a table, only + the name, id and hash pointers may be accessed. @param id table ID @return table @retval nullptr if not cached */ - dict_table_t *find_table(table_id_t id) const noexcept; + dict_table_t *find_table_any(table_id_t id) const noexcept; + + /** Look up a persistent table, waiting for a concurrent load of it + to finish if necessary. + @param id table ID + @param excl whether the caller holds dict_sys.latch in exclusive + mode (lock()/unlock()); if false, the caller holds it + in shared mode (freeze()/unfreeze()) + @return table + @retval nullptr if not cached, or a concurrent load of it failed */ + dict_table_t *find_table(table_id_t id, bool excl) noexcept; bool is_initialised() const noexcept { return m_initialised; } @@ -1416,7 +1428,7 @@ class dict_sys_t /** Move a table to the non-LRU list from the LRU list. */ void prevent_eviction(dict_table_t *table) { - ut_d(locked()); + ut_ad(locked()); ut_ad(find(table)); if (!table->can_be_evicted) return; @@ -1425,6 +1437,20 @@ class dict_sys_t UT_LIST_ADD_LAST(table_non_LRU, table); } + /** Move a table to the LRU list from the non-LRU list. + This is the inverse of prevent_eviction(), invoked by + dict_load_table_one() when a fully loaded table replaces + its non-evictable loading stub. */ + void allow_eviction(dict_table_t *table) + { + ut_ad(locked()); + ut_ad(find(table)); + ut_ad(!table->can_be_evicted); + table->can_be_evicted= true; + UT_LIST_REMOVE(table_non_LRU, table); + UT_LIST_ADD_FIRST(table_LRU, table); + } + #ifdef UNIV_DEBUG /** @return whether the current thread is holding the latch */ bool frozen() const noexcept { return latch.have_any(); } @@ -1488,17 +1514,62 @@ class dict_sys_t @return number of tables evicted */ ulint evict_table_LRU(bool half) noexcept; - /** Look up a table in the dictionary cache. + /** Look up a table in the dictionary cache, including tables whose + definition is still being loaded (dict_table_t::loading()). Of such + a table, only the name, id and hash pointers may be accessed. @param name table name @return table handle @retval nullptr if not found */ - dict_table_t *find_table(const span &name) const noexcept; + dict_table_t *find_table_any(const span &name) const noexcept; - /** Look up or load a table definition + /** Look up a table in the dictionary cache, waiting for a + concurrent load of it to finish if necessary. + @param name table name + @param excl whether the caller holds dict_sys.latch in exclusive + mode (lock()/unlock()); if false, the caller holds it + in shared mode (freeze()/unfreeze()) + @return table handle + @retval nullptr if not found, or a concurrent load of it failed */ + dict_table_t *find_table(const span &name, bool excl) noexcept; + + /** Look up a table for FOREIGN KEY constraint linkage. + Like find_table(), except that a table whose definition is complete + and that is only waiting for the tables related to it by FOREIGN KEY + constraints to be loaded (dict_table_t::LOADING_FK) is visible: + linking constraints into such a table is protected by the exclusive + latch, which both the linking thread and the loading thread hold + while modifying foreign_set/referenced_set. @param name table name - @param ignore errors to ignore when loading the table definition @return table handle @retval nullptr if not found */ + dict_table_t *find_table_fk(const span &name) const noexcept; + + /** Wait for a concurrent dict_load_table_one() of a specific table + to finish. To be invoked with the latch held in the given mode, + after observing a table with dict_table_t::loading() set; releases + and reacquires the latch, in that same mode, around the wait. + @param table a table observed to be loading; must not be nullptr + @param excl whether the caller holds the latch in exclusive mode + (lock()/unlock()); if false, the caller holds it in + shared mode (freeze()/unfreeze()) + @return whether the load succeeded; the latch is held on return + either way, in the mode the caller held it in, so on success table + is still valid and usable + @retval false if the load failed; table may already have been freed */ + bool wait_for_load(dict_table_t *table, bool excl) noexcept; + + /** Look up or load a table definition. The table and any tables + that were loaded because they are related to it by FOREIGN KEY + constraints become visible to other threads atomically, only after + all of them have been loaded. + @param name table name + @param ignore errors to ignore when loading the table definition + @return table handle + @retval nullptr if not found + @note The exclusive latch may be temporarily released and reacquired, + both while waiting for a concurrent load of the same table and during + the I/O phases of loading; any pointers into the cache that the + caller obtained earlier may be stale on return. */ dict_table_t *load_table(const span &name, dict_err_ignore_t ignore= DICT_ERR_IGNORE_NONE) noexcept; diff --git a/storage/innobase/include/dict0dict.inl b/storage/innobase/include/dict0dict.inl index fa2ba394bdbb4..0fd57d499e2dc 100644 --- a/storage/innobase/include/dict0dict.inl +++ b/storage/innobase/include/dict0dict.inl @@ -1076,8 +1076,8 @@ dict_table_is_file_per_table( /** Acquire the table handle. */ inline void dict_table_t::acquire() { - ut_d(const auto old=) n_ref_count++; - ut_ad(old || dict_sys.frozen()); + ut_d(const auto old=) n_ref_count.fetch_add(1, std::memory_order_relaxed); + ut_ad((old & ~LOADING_MASK) || dict_sys.frozen()); } /** Release the table handle. @@ -1086,11 +1086,33 @@ inline bool dict_table_t::release() { - auto n = n_ref_count--; - ut_ad(n > 0); + auto n = n_ref_count.fetch_sub(1, std::memory_order_relaxed); + ut_ad(n & ~LOADING_MASK); return n == 1; } +/** Clear loading(), and release the exclusive lock_latch taken for +the load, making the table fully visible. */ +inline void dict_table_t::load_finish() noexcept +{ + ut_ad(dict_sys.locked()); + ut_ad(is_loader()); + ut_d(const auto old=) + n_ref_count.fetch_sub(LOADING_FK, std::memory_order_relaxed); + ut_ad((old & LOADING_MASK) == LOADING_FK); + lock_mutex_unlock(); +} + +/** Mark a failed load, and release the exclusive lock_latch taken +for it. */ +inline void dict_table_t::mark_load_failed() noexcept +{ + ut_ad(dict_sys.locked()); + ut_ad(is_loader()); + n_ref_count.fetch_or(LOAD_FAILED, std::memory_order_relaxed); + lock_mutex_unlock(); +} + /** Encode the number of columns and number of virtual columns in a 4 bytes value. We could do this because the number of columns in InnoDB is limited to 1017 diff --git a/storage/innobase/include/dict0load.h b/storage/innobase/include/dict0load.h index aec0b992e503b..b6e382957d408 100644 --- a/storage/innobase/include/dict0load.h +++ b/storage/innobase/include/dict0load.h @@ -98,6 +98,11 @@ dict_load_foreigns( bool check_charsets, /*!< in: whether to check charset compatibility */ dict_err_ignore_t ignore_err, /*!< in: error to be ignored */ + mem_heap_t* fk_heap, /*!< in: heap on which the + names appended to fk_tables + are allocated; must remain + valid until fk_tables has + been drained by the caller */ dict_names_t& fk_tables) /*!< out: stack of table names which must be loaded subsequently to load all the diff --git a/storage/innobase/include/dict0mem.h b/storage/innobase/include/dict0mem.h index 8fbbd1fb744ac..6dbf641d7162d 100644 --- a/storage/innobase/include/dict0mem.h +++ b/storage/innobase/include/dict0mem.h @@ -1928,9 +1928,13 @@ typedef enum { zero-initialized in dict_table_t::create(). */ struct dict_table_t { - /** Get reference count. - @return current value of n_ref_count */ - inline uint32_t get_ref_count() const { return n_ref_count; } + /** Get reference count. The caller must already know the table + is not loading(), typically because it was obtained via + find_table() or load_table(); a caller that may see a loading + table must mask off LOADING_MASK itself. + @return current value of the reference count */ + inline uint32_t get_ref_count() const + { return n_ref_count.load(std::memory_order_relaxed); } /** Acquire the table handle. */ inline void acquire(); @@ -2261,6 +2265,85 @@ struct dict_table_t { or ONLINE_INDEX_ABORTED_DROPPED. */ unsigned drop_aborted:1; + /** columns/indexes are being loaded by dict_load_table_one(); + only the loading thread may access the object, which holds + lock_latch in exclusive mode */ + static constexpr uint32_t LOADING_DEF= 1U << 30; + /** the definition is complete; only the FOREIGN KEY related + tables are still being loaded by dict_sys_t::load_table(). + find_table_fk() exposes the table so constraints can be linked + into it under the exclusive dict_sys.latch */ + static constexpr uint32_t LOADING_FK= 2U << 30; + /** the load failed; the stub is unlinked once every reference + taken before the failure (see try_pin_for_wait()) is released */ + static constexpr uint32_t LOAD_FAILED= 3U << 30; + /** mask of LOADING_DEF|LOADING_FK|LOAD_FAILED within n_ref_count */ + static constexpr uint32_t LOADING_MASK= 3U << 30; + + /** @return the progress of loading the table definition, 0 if + fully loaded */ + uint32_t loading() const noexcept + { return n_ref_count.load(std::memory_order_relaxed) & LOADING_MASK; } + +#ifdef UNIV_DEBUG + /** The thread that set the loading flag */ + pthread_t load_thread; + + /** @return whether the current thread is loading this table */ + bool is_loader() const + { return loading() && pthread_equal(pthread_self(), load_thread); } +#endif + + /** Try to pin the table only to wait on lock_latch for a + concurrent load to finish; refuses once LOAD_FAILED, so that a + late pin cannot defeat the loader's drain to zero references. + @return whether a reference was acquired */ + bool try_pin_for_wait() noexcept + { + uint32_t n= n_ref_count.load(std::memory_order_relaxed); + while (!((n & LOADING_MASK) == LOAD_FAILED)) + if (n_ref_count.compare_exchange_weak(n, n + 1, + std::memory_order_relaxed, + std::memory_order_relaxed)) + return true; + return false; + } + + /** Mark a freshly created, not yet published table as loading. */ + void start_loading() noexcept + { + ut_ad(!n_ref_count.load(std::memory_order_relaxed)); + n_ref_count.store(LOADING_DEF, std::memory_order_relaxed); + } + + /** Advance loading() from LOADING_DEF to LOADING_FK. */ + void advance_to_loading_fk() noexcept + { + ut_d(const auto old=) + n_ref_count.fetch_add(LOADING_FK - LOADING_DEF, + std::memory_order_relaxed); + ut_ad((old & LOADING_MASK) == LOADING_DEF); + } + + /** Clear loading(), and release the exclusive lock_latch taken + for the load, making the table fully visible. Defined in + dict0dict.inl, where dict_sys is visible for the assertions. */ + void load_finish() noexcept; + + /** Mark a failed load, and release the exclusive lock_latch taken + for it. Defined in dict0dict.inl, where dict_sys is visible for + the assertions. */ + void mark_load_failed() noexcept; + + /** @return whether every reference taken before mark_load_failed() + (see try_pin_for_wait()) has been released, so the stub can be + freed. Only to be called after mark_load_failed(). */ + bool load_failed_refs_cleared() const noexcept + { + ut_ad(loading() == LOAD_FAILED); + return n_ref_count.load(std::memory_order_relaxed) == LOAD_FAILED; + } + /** Array of column descriptions. */ dict_col_t* cols; @@ -2411,9 +2494,13 @@ struct dict_table_t { #else typedef srw_spin_lock_low lock_latch_type; #endif - /** RW-lock protecting locks and statistics on this table */ + /** RW-lock protecting locks and statistics on this table. + Also held in exclusive mode by dict_load_table_one() while loading() + is set, so that dict_sys_t::load_table() can wait for a concurrent + load of this table by acquiring it in shared mode. */ lock_latch_type lock_latch; public: + /** The next DB_ROW_ID value */ Atomic_counter row_id{0}; /** Autoinc counter value to give to the next inserted row. */ @@ -2456,10 +2543,13 @@ struct dict_table_t { @see trx_lock_t::trx_locks */ Atomic_counter n_rec_locks; private: - /** Count of how many handles are opened to this table. Dropping of the - table is NOT allowed until this count gets to zero. MySQL does NOT - itself check the number of open handles at DROP. */ - Atomic_counter n_ref_count; + /** Count of how many handles are opened to this table, in the low + 30 bits; the top 2 bits hold loading() (see above). Dropping of the + table is NOT allowed until the reference count reaches zero. MySQL + does NOT itself check the number of open handles at DROP. + The two live in one atomic so that acquire()/release() are + unconditional and never touch the top bits. */ + std::atomic n_ref_count; public: /** List of locks on the table. Protected by lock_sys.assert_locked(lock). */ table_lock_list_t locks; diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc index 2f6585032c253..c6a33d28c197c 100644 --- a/storage/innobase/lock/lock0lock.cc +++ b/storage/innobase/lock/lock0lock.cc @@ -4705,7 +4705,7 @@ void lock_release(trx_t *trx) dict_sys.lock(SRW_LOCK_CALL); LockMutexGuard g{SRW_LOCK_CALL}; for (const table_id_t id : to_evict) - if (dict_table_t *table= dict_sys.find_table(id)) + if (dict_table_t *table= dict_sys.find_table(id, true)) if (!table->get_ref_count() && !UT_LIST_GET_LEN(table->locks)) dict_sys.remove(table, true); dict_sys.unlock(); diff --git a/storage/innobase/row/row0merge.cc b/storage/innobase/row/row0merge.cc index b5d43d5c3c5a2..14a13b6f32920 100644 --- a/storage/innobase/row/row0merge.cc +++ b/storage/innobase/row/row0merge.cc @@ -4083,6 +4083,7 @@ row_merge_drop_indexes( ut_ad(dict_index_get_online_status(index) == ONLINE_INDEX_COMPLETE); /* the caller should have an open handle to the table */ + ut_ad(!table->loading()); ut_ad(table->get_ref_count() >= 1); /* It is possible that table->n_ref_count > 1 when diff --git a/storage/innobase/row/row0mysql.cc b/storage/innobase/row/row0mysql.cc index 4b99a5756cc43..7111d4540d493 100644 --- a/storage/innobase/row/row0mysql.cc +++ b/storage/innobase/row/row0mysql.cc @@ -2834,6 +2834,12 @@ row_rename_table_for_mysql( /* We only want to switch off some of the type checking in an ALTER TABLE, not in a RENAME. */ dict_names_t fk_tables; + /* The names in fk_tables must survive the temporary + release of the exclusive dict_sys.latch inside + dict_sys.load_table() in the drain loop below. */ + if (!heap) { + heap = mem_heap_create(1000); + } { mtr_t mtr{trx}; err = dict_load_foreigns(mtr, new_name, nullptr, @@ -2843,7 +2849,7 @@ row_rename_table_for_mysql( fk == RENAME_ALTER_COPY ? DICT_ERR_IGNORE_NONE : DICT_ERR_IGNORE_FK_NOKEY, - fk_tables); + heap, fk_tables); } if (err != DB_SUCCESS) { if (old_is_tmp) { diff --git a/storage/innobase/row/row0purge.cc b/storage/innobase/row/row0purge.cc index 1991dbb870575..f428dd60aee7a 100644 --- a/storage/innobase/row/row0purge.cc +++ b/storage/innobase/row/row0purge.cc @@ -117,7 +117,7 @@ row_purge_remove_clust_if_poss_low( if (table_id) { retry: dict_sys.lock(SRW_LOCK_CALL); - table = dict_sys.find_table(table_id); + table = dict_sys.find_table(table_id, true); if (!table) { dict_sys.unlock(); } else if (table->n_rec_locks) { @@ -167,6 +167,7 @@ row_purge_remove_clust_if_poss_low( &node->pcur, nullptr, &mtr); if (space_id) { if (table) { + ut_ad(!table->loading()); if (table->get_ref_count() == 0) { dict_sys.remove(table); } else if (table->space_id == space_id) { diff --git a/storage/innobase/trx/trx0purge.cc b/storage/innobase/trx/trx0purge.cc index 02706abb8e50d..e15b41cf01a9f 100644 --- a/storage/innobase/trx/trx0purge.cc +++ b/storage/innobase/trx/trx0purge.cc @@ -1190,7 +1190,7 @@ static purge_table trx_purge_table_open(table_id_t table_id, for (;;) { dict_sys.freeze(SRW_LOCK_CALL); - result.table= dict_sys.find_table(table_id); + result.table= dict_sys.find_table(table_id, false); if (result.table) break; dict_sys.unfreeze(); diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc index e95df263ff278..125cc14d61e9b 100644 --- a/storage/innobase/trx/trx0trx.cc +++ b/storage/innobase/trx/trx0trx.cc @@ -1304,13 +1304,14 @@ void trx_t::evict_table(table_id_t table_id, bool reset_only) { ut_ad(in_rollback); - dict_table_t* table = dict_sys.find_table(table_id); + dict_table_t* table = dict_sys.find_table(table_id, true); if (!table) { return; } table->def_trx_id = 0; + ut_ad(!table->loading()); if (auto ref_count = table->get_ref_count()) { /* This must be a DDL operation that is being rolled back in an active connection. */ diff --git a/storage/innobase/trx/trx0undo.cc b/storage/innobase/trx/trx0undo.cc index 611863d5e2415..503144c14ce67 100644 --- a/storage/innobase/trx/trx0undo.cc +++ b/storage/innobase/trx/trx0undo.cc @@ -306,7 +306,7 @@ void UndorecApplier::apply_undo_rec(const trx_undo_rec_t *rec, uint16_t offset) &cmpl_info, &updated_extern, &undo_no, &table_id); dict_sys.freeze(SRW_LOCK_CALL); - dict_table_t *table= dict_sys.find_table(table_id); + dict_table_t *table= dict_sys.find_table(table_id, false); dict_sys.unfreeze(); ut_ad(table);