Repository navigation
MDEV-35154 : dict_sys_t::load_table() is holding exclusive dict_sys.latch for unnecessarily long time - #5606
mariadb-TafzeelShams wants to merge 1 commit into
Conversation
| /* 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. */ | ||
| fk_heap = mem_heap_create(1000); |
There was a problem hiding this comment.
I wonder if heap could be reused instead.
| because it is checked by cache lookups while the loading thread | ||
| is modifying the bit-fields without holding any latch. | ||
| Zero-initialized (NOT_LOADING) by dict_table_t::create(). */ | ||
| Atomic_relaxed<byte> loading; |
There was a problem hiding this comment.
Could we consider using an enum (byte-sized) instead? Also, could you please check with gdb/pahole if this field can fit (or already does) in existing holes and does not bloat the struct size?
| { | ||
| /* A table whose definition is still being loaded must never be | ||
| referenced; dict_sys_t::find_table() hides such tables. */ | ||
| ut_ad(!loading); |
There was a problem hiding this comment.
Note: this relies on the "initial state" of the loading field being not 0 == dict_table_t::NOT_LOADING.
There was a problem hiding this comment.
It would be better to write NOT_LOADING == loading. The emitted code should be the same, except for the assertion string.
However, isn’t this an unnecessarily strict assumption? What is the issue with other threads holding references to tables while they are being loaded? Is it one where the table definition is found to be corrupted and we decide to free the object? If so, that scenario should be manageable with further possible values of loading.
My idea for this was that a thread would look up the table and acquire() a reference unless the table is in the process of being evicted from the cache (identified by a new loading value). In case loading indicates that a thread is loading the table definition (and holding exclusive lock_latch), we will acquire a shared lock_latch. Once the loader completes, it’ll update the loading status (in case the table needs to be evicted) and release the lock_latch. Then, the threads that were waiting would reload loading. If the table is to be evicted, they will release the reference. Finally, they release their shared lock_latch on the table. If the table needs to be evicted, the original "loading" thread would wait for the reference count to reach 0 before removing the table definition. The reference-count does need to be re-checked after acquiring exclusive dict_sys.latch.
| the caller will observe the stale stub again and simply wait again. */ | ||
| timespec abstime; | ||
| set_timespec(abstime, 5); | ||
| my_cond_timedwait(&load_cond, &load_mutex.m_mutex, &abstime); |
There was a problem hiding this comment.
As @dr-m pointed out in an earlier PR, this synchronization mechanism has a great memory cost in the data structure. That is mitigated by the fact that we have only one instantiation of dict_sys_t I guess. Also, we're likely waiting on I/O here, so I believe lightweight futex-based mutexes and retry-loop may be even worse.
| /* The entry may be a stub that another thread is loading; | ||
| check the atomic flag before the bit-field to avoid a | ||
| torn read. */ | ||
| //ut_ad((*after)->loading || (*after)->cached); |
There was a problem hiding this comment.
Why this is commented out?
| constraints are loaded. | ||
| @return table, possibly with file_unreadable flag set | ||
| @retval nullptr if the table does not exist */ | ||
| /** Determine whether a table must be loaded without ever releasing |
There was a problem hiding this comment.
I think this dict_load_hold_latch() has slipped between dict_load_table_one() below and its docstring above.
|
|
||
| const char* n = static_cast<const char*>(p) + 1; | ||
|
|
||
| if (name.end() - n > 4 |
There was a problem hiding this comment.
Maybe name.end() > n + 4 is safer?
| err_exit: | ||
| mtr.commit(); | ||
| if (uncommitted) { | ||
| /* A stub was removed at reload:; wake up any |
There was a problem hiding this comment.
Pull request overview
Reduces global dictionary latch contention during InnoDB table loading by publishing guarded loading stubs and performing I/O outside the latch.
Changes:
- Adds staged, concurrent dictionary loading with wait signaling.
- Preserves foreign-key loading consistency and table lifetimes.
- Adds concurrency and foreign-key regression coverage.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
storage/innobase/row/row0mysql.cc |
Preserves FK names during rename loading. |
storage/innobase/include/univ.i |
Declares load-mutex instrumentation. |
storage/innobase/include/dict0mem.h |
Adds table-loading states. |
storage/innobase/include/dict0load.h |
Extends FK loading API. |
storage/innobase/include/dict0dict.inl |
Guards acquisition of incomplete tables. |
storage/innobase/include/dict0dict.h |
Adds loading lookup and synchronization APIs. |
storage/innobase/handler/handler0alter.cc |
Preserves ALTER FK-name storage. |
storage/innobase/handler/ha_innodb.cc |
Handles table lifetimes across latch releases. |
storage/innobase/dict/dict0load.cc |
Implements phased concurrent loading. |
storage/innobase/dict/dict0dict.cc |
Implements loading-aware lookups and waits. |
storage/innobase/btr/btr0sea.cc |
Skips incomplete tables during AHI shutdown. |
mysql-test/suite/innodb/t/dict_load_concurrent.test |
Tests concurrent loading and FK visibility. |
mysql-test/suite/innodb/r/dict_load_concurrent.result |
Records expected regression-test output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| referenced_set. The table would become | ||
| non-evictable anyway when the constraint is added | ||
| to the dictionary cache. */ | ||
| dict_sys.prevent_eviction(foreign->referenced_table); |
| latch.SRW_LOCK_INIT(dict_operation_lock_key); | ||
| mysql_mutex_init(dict_load_mutex_key, &load_mutex, nullptr); | ||
| pthread_cond_init(&load_cond, nullptr); |
There was a problem hiding this comment.
Do we really need these new and unnecessarily large synchronisation primitives? I think that I suggested using dict_table_t::lock_latch. It would improve the locality of reference and avoid a central bottleneck or spurious wakeups caused by dict_sys.load_done().
| because it is checked by cache lookups while the loading thread | ||
| is modifying the bit-fields without holding any latch. | ||
| Zero-initialized (NOT_LOADING) by dict_table_t::create(). */ | ||
| Atomic_relaxed<byte> loading; |
There was a problem hiding this comment.
The field cached should be merged to this one. We might want a special (nonzero, debug-only) value for "not cached" for any dict_table_t that are not attached to dict_sys.
| mem_heap_t* heap = NULL; | ||
| mem_heap_t* fk_heap = NULL; |
There was a problem hiding this comment.
We could use heap for both uses.
| { | ||
| /* A table whose definition is still being loaded must never be | ||
| referenced; dict_sys_t::find_table() hides such tables. */ | ||
| ut_ad(!loading); |
There was a problem hiding this comment.
It would be better to write NOT_LOADING == loading. The emitted code should be the same, except for the assertion string.
However, isn’t this an unnecessarily strict assumption? What is the issue with other threads holding references to tables while they are being loaded? Is it one where the table definition is found to be corrupted and we decide to free the object? If so, that scenario should be manageable with further possible values of loading.
My idea for this was that a thread would look up the table and acquire() a reference unless the table is in the process of being evicted from the cache (identified by a new loading value). In case loading indicates that a thread is loading the table definition (and holding exclusive lock_latch), we will acquire a shared lock_latch. Once the loader completes, it’ll update the loading status (in case the table needs to be evicted) and release the lock_latch. Then, the threads that were waiting would reload loading. If the table is to be evicted, they will release the reference. Finally, they release their shared lock_latch on the table. If the table needs to be evicted, the original "loading" thread would wait for the reference count to reach 0 before removing the table definition. The reference-count does need to be re-checked after acquiring exclusive dict_sys.latch.
f033f5a to
e53d179
Compare
8ba7981 to
698901b
Compare
There was a problem hiding this comment.
This looks like an unintentional change of the submodule. Beware of git commit -a.
| if (table->loading()) { | ||
| /* The table definition is being loaded by another | ||
| thread without dict_sys.latch; walking its indexes | ||
| would race with that thread. Its indexes cannot have | ||
| any adaptive hash index references. */ | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The logic is correct, but the last sentence should be more specific:
The adaptive hash index can only point to records of indexes whose metadata is already cached.
| /* A table that is still being loaded is treated as if it | ||
| were not cached. */ | ||
| if (t->loading()) | ||
| return false; |
There was a problem hiding this comment.
Pretending that the entry does not exist is likely to cause a further hash table lookup, likely with another acquisition and release of dict_sys.latch.
We should make it a responsibility of the caller to check dict_table_t::loading() and act accordingly. They should acquire a reference, release dict_sys.latch, and then check dict_table_t::loading(). If it is set, acquire the table latch, check if the table is valid, and release the latch. If the table is invalid, release the reference to let the "loader thread" proceed to free the object.
There was a problem hiding this comment.
In 17caf5f
find_table() (both by-id and by-name) no longer hides a loading table — it waits for the concurrent load via wait_for_load() and only returns nullptr if the table is genuinely absent or the load failed, avoiding the extra hash lookup.
| dict_table_t *dict_sys_t::find_table(const span<const char> &name) | ||
| const noexcept | ||
| { | ||
| dict_table_t *t= find_table_any(name); | ||
| /* A table whose loading has not completed must not be accessed; | ||
| treat it as if it were not cached. */ | ||
| return t && t->loading() ? nullptr : t; | ||
| } |
There was a problem hiding this comment.
Similar to the other dict_sys_t::find_table(), I think that we must require each caller to check the dict_table_t::loading() status.
| /* 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; |
There was a problem hiding this comment.
Unlike the changes to dict_sys_t::find_table(), this one may be unavoidable.
| if (name.end() > n + 4 | ||
| && (n[0] == 'F' || n[0] == 'f') | ||
| && (n[1] == 'T' || n[1] == 't') | ||
| && (n[2] == 'S' || n[2] == 's') | ||
| && n[3] == '_') { | ||
| /* FULLTEXT INDEX auxiliary tables */ | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I don’t think I ever saw the FTS_ names in lower case. Do we really want to allow that, as well as mixed-case like FtS_? What if a user creates a table whose name starts like this? Why do some internal tables have to be treated differently? Can we avoid that?
There was a problem hiding this comment.
Why do some internal tables have to be treated differently? Can we avoid that?
Tables that InnoDB internal SQL can refer to are loaded without ever releasing the latch. The internal SQL parser is not reentrant and is serialized only by the exclusive dict_sys.latch, and it opens tables in the middle of parsing; releasing the latch there let another thread run the parser concurrently and corrupt its state.
| evict: | ||
| dict_load_table_one_discard(table); | ||
| mem_heap_free(heap); |
There was a problem hiding this comment.
The heap is a temporary heap for constructing a string of column names or for constructing READ COMMITTED versions of records of SYS_COLUMNS, SYS_INDEXES, or SYS_FIELDS. There is no reason to postpone the freeing.
| /* Another thread is loading the table definition. Wait for it | ||
| to complete or fail, and retry the lookup: the stub may have | ||
| been removed (and freed) by the time we wake up. */ | ||
| ut_ad(!table->is_loader()); | ||
| wait_for_load(table); |
There was a problem hiding this comment.
Why retry the lookup? wait_for_load() should return a final status.
There was a problem hiding this comment.
Done, wait_for_load() now returns bool. On success it reacquires dict_sys.latch before releasing its own pin, so the table is still valid to return directly. The lookup is only repeated when the load actually failed.
| auto n = n_ref_count--; | ||
| ut_ad(n > 0); | ||
| return n == 1; | ||
| auto n = n_ref_count.fetch_sub(1, std::memory_order_relaxed); | ||
| ut_ad(n & ~LOADING_MASK); | ||
| return (n & ~LOADING_MASK) == 1; |
There was a problem hiding this comment.
Is there really a reason to change the return expression? There are only two callers that care about the return value. In neither of the callers should we allow loading() to hold.
There was a problem hiding this comment.
Simplified to return n == 1;
| /** Get reference count. | ||
| @return current value of n_ref_count */ | ||
| inline uint32_t get_ref_count() const { return n_ref_count; } | ||
| @return current value of the reference count part of n_ref_count */ | ||
| inline uint32_t get_ref_count() const | ||
| { return n_ref_count.load(std::memory_order_relaxed) & ~LOADING_MASK; } |
There was a problem hiding this comment.
Do we really want the additional bitmask here? There are few non-assertion callers who are interested in the value being 0, 1, or more. Can each of them assert !table->loading() before invoking this?
There was a problem hiding this comment.
Done. get_ref_count() no longer masks, and ut_ad(!table->loading()) is added before most of the callers.
dict_sys_t::remove() masks the bit because it also runs right after a failed load, and innodb_drop_database() masks it because it walks the table hash directly. Both are documented at the call site.
698901b to
9007418
Compare
9007418 to
17caf5f
Compare
…atch for unnecessarily long time Issue: dict_load_table_one() was invoked with the exclusive dict_sys.latch held and never released it. The whole load ran inside that one critical section: reading the SYS_TABLES record, opening the .ibd file, and reading SYS_COLUMNS, SYS_VIRTUAL, SYS_INDEXES, SYS_FIELDS, the clustered index root page, and SYS_FOREIGN, as well as loading every table related by FOREIGN KEY constraints. Almost all of that is I/O. Because dict_sys.latch is the single latch that guards every dictionary lookup, one cold table open blocked every other session from opening any table, including tables that were already cached. A slow enough load could trigger the fatal semaphore wait check. Fix: Split the load into three phases. A short latched phase creates the table object and publishes it as an incomplete "stub"; the I/O runs with no latch held; a final latched phase links the FOREIGN KEY constraints. The stub's progress is held in the top two bits of dict_table_t:: n_ref_count (LOADING_DEF, LOADING_FK, LOAD_FAILED), analogous to how buf_page_t::state() combines a small lifecycle state with the buffer-fix count in one atomic. A thread that finds a loading table waits by pinning it (dict_table_t::try_pin_for_wait(), so it cannot be freed while unlatched) and acquiring dict_table_t::lock_latch in shared mode, the same latch the loader holds in exclusive mode for the duration of the load. dict_sys_t::wait_for_load() reports whether the load it waited for actually succeeded, so a caller that only needed to wait for the table it already found can use it directly afterwards instead of repeating the lookup; a caller that only wanted to confirm a related table had finished linking its constraints can likewise skip straight to treating it as already linked. A failed load can be torn down safely even though other threads may have already taken such a pin to wait on it: try_pin_for_wait() refuses once LOAD_FAILED is set, and the teardown path drains any pins taken earlier before freeing the stub. A table remains hidden (LOADING_FK) until every table related to it by FOREIGN KEY constraints has been loaded as well, and dict_sys_t:: load_table() makes them all visible in one step. The intermediate state is visible to constraint linking via dict_sys_t::find_table_fk(), so that two threads loading tables that reference each other can still link the constraint between them. Tables that InnoDB internal SQL can refer to are loaded without ever releasing the latch. The internal SQL parser is not reentrant and is serialized only by the exclusive dict_sys.latch, and it opens tables in the middle of parsing; releasing the latch there let another thread run the parser concurrently and corrupt its state. Changes: - dict_table_t : hold the progress of loading in the top bits of n_ref_count (std::atomic<uint32_t>): LOADING_DEF, LOADING_FK, LOAD_FAILED, loading(). Add the loader-only helpers start_loading(), advance_to_loading_fk(), load_finish(), mark_load_failed(), and try_pin_for_wait() for waiters, all built on the existing lock_latch. - dict_load_table_one() : publish the table as a LOADING_DEF stub in table_non_LRU and release the latch before loading the tablespace, the columns and the indexes; reacquire it, move the table to table_LRU and advance it to LOADING_FK before loading the foreign key constraints. - dict_sys_t::load_table() : wait for a concurrent load of the same table via wait_for_load(); allocate the foreign key table names on a local heap, because the latch is released while draining them; in the drain loop, wait for a table whose definition is still being loaded by another thread and skip one that is only waiting for its own related tables; make all tables loaded by this invocation visible in one step via load_finish(). - dict_sys_t::wait_for_load() : pins the observed table via try_pin_for_wait() (returning false immediately if the load already failed), releases dict_sys.latch, blocks on the table's own lock_latch, reacquires dict_sys.latch, and only then checks loading() and releases its pin, returning whether the load succeeded. Because the latch is reacquired before the pin is released, a successful wait leaves the table valid to use directly, with no need to repeat the lookup that found it. Takes an excl parameter: whether the caller holds dict_sys.latch in exclusive mode (lock()/unlock()) or shared mode (freeze()/unfreeze()), so it knows which pair to use to release and reacquire it. - dict_load_table_one_discard() : used wherever a stub must be torn down (retry after DB_SUCCESS_LOCKED_REC, column or virtual-column load failure, a corrupted index or missing FK index). Calls mark_load_failed(), which marks the stub LOAD_FAILED and releases lock_latch to wake any already-pinned waiters, then drains the reference count to zero before removing the stub. - dict_sys_t::add() : take lock_latch in exclusive mode on the loader's behalf before a loading stub becomes reachable via find_table_any(). - dict_get_and_save_data_dir_path() : skip re-acquiring lock_latch when the table is loading(), because the loading thread already holds it in exclusive mode and is the only thread that can reach the table at that point. - dict_load_hold_latch() : whether a table may be referenced by InnoDB internal SQL, and therefore must be loaded without releasing the latch: InnoDB system tables, FULLTEXT INDEX auxiliary tables and the persistent statistics tables. The FULLTEXT prefix match is case-sensitive, matching how these auxiliary table names are actually generated. - dict_load_table_on_id() : copy the table name and release the SYS_TABLES page latch before calling load_table(), which may now block. Restore the cursor position only if the scan has to continue. - dict_load_foreign(), dict_load_foreigns() : add the fk_heap parameter and allocate the names appended to fk_tables on it. - dict_sys_t::find_table_any() : an unconditional lookup by table name or table_id_t, returning a table even while it is still being loaded. Only the name (or id) and hash pointers may be read on such a table. - dict_sys_t::find_table() : wait for a concurrent load of the table it finds, via wait_for_load(), both in the by-name and the by-id variant, retrying the lookup if the load failed, since the stub may have been freed, or a fresh load of the same name or id may now be in progress. Takes an excl parameter so that the caller can state which dict_sys.latch mode it holds: dict_acquire_mdl_shared(), dict_table_open_on_id(), dict_table_open_on_name(), row_purge_remove_clust_if_poss_low(), trx_t::evict_table(), UndorecApplier::apply_undo_rec(), trx_purge_table_open(), prepare_inplace_alter_table_dict(), lock_release(), and create_table_info_t::create_foreign_keys() all pass it. - dict_sys_t::find_table_fk() : like find_table(), but LOADING_FK tables are returned, so that constraints can be linked into them while holding the exclusive latch. - dict_table_can_be_evicted() : a table that is being loaded may only be freed by the thread that is loading it. - dict_foreign_add_to_cache() : resolve both sides with find_table_fk(). - dict_table_t::load_failed_refs_cleared() : whether every reference taken before a failed load has been released. dict_load_table_one_discard() uses this to drain a failed stub before freeing it, instead of comparing get_ref_count() to 0. - create_table_info_t::create_foreign_keys() : take a temporary reference on a referenced table as soon as it is resolved, released on every exit path by a scope guard; call dict_sys.prevent_eviction() only once the constraint is actually committed to the dictionary cache, replacing the temporary reference. - create_table_info_t::create_table() : acquire a reference to the created table around the foreign key handling, and use a local heap for the names of the foreign key related tables. - assertion fix : the load path may now run without dict_sys.latch, but only in the thread that is loading the table, and a cached table is no longer necessarily fully loaded. dict_sys.locked() is relaxed to "dict_sys.locked() || table->is_loader()" in dict_load_columns(), dict_load_virtual_col(), dict_load_fields(), dict_load_indexes(), dict_index_add_to_cache(), dict_index_find_cols(), dict_index_build_internal_clust(), dict_index_build_internal_non_clust() and dict_index_build_internal_fts(). Checks of dict_table_t::cached are relaxed and reordered after the atomic loading in dict_table_add_system_columns(), dict_sys_t::add(), dict_table_rename_in_cache() and hash_insert(). - innodb.dict_load_concurrent A load is parked at dict_load_table_one_no_latch while holding no latch; another table can be loaded meanwhile, and a second opener of the same table waits. A second case checks that a table is not made visible while a table related to it by a FOREIGN KEY constraint is still being loaded by another thread.
17caf5f to
08377b6
Compare
Description
Issue:
dict_load_table_one() was invoked with the exclusive dict_sys.latch held
and never released it. The whole load ran inside that one critical
section: reading the SYS_TABLES record, opening the .ibd file, and
reading SYS_COLUMNS, SYS_VIRTUAL, SYS_INDEXES, SYS_FIELDS, the clustered
index root page, and SYS_FOREIGN, as well as loading every table related
by FOREIGN KEY constraints.
Almost all of that is I/O. Because dict_sys.latch is the single latch
that guards every dictionary lookup, one cold table open blocked every
other session from opening any table, including tables that were already
cached. A slow enough load could trigger the fatal semaphore wait check.
Fix:
Split the load into three phases. A short latched phase creates the
table object and publishes it as an incomplete "stub"; the I/O runs with
no latch held; a final latched phase links the FOREIGN KEY constraints.
The stub's progress is held in the top two bits of dict_table_t::
n_ref_count (LOADING_DEF, LOADING_FK, LOAD_FAILED), analogous to how
buf_page_t::state() combines a small lifecycle state with the buffer-fix
count in one atomic. A thread that finds a loading table waits by
pinning it (dict_table_t::try_pin_for_wait(), so it cannot be freed while
unlatched) and acquiring dict_table_t::lock_latch in shared mode, the
same latch the loader holds in exclusive mode for the duration of the
load. dict_sys_t::wait_for_load() reports whether the load it waited for
actually succeeded, so a caller that only needed to wait for the table it
already found can use it directly afterwards instead of repeating the
lookup; a caller that only wanted to confirm a related table had finished
linking its constraints can likewise skip straight to treating it as
already linked. A failed load can be torn down safely even though other
threads may have already taken such a pin to wait on it: try_pin_for_wait()
refuses once LOAD_FAILED is set, and the teardown path drains any pins
taken earlier before freeing the stub.
A table remains hidden (LOADING_FK) until every table related to it by
FOREIGN KEY constraints has been loaded as well, and dict_sys_t::
load_table() makes them all visible in one step. The intermediate state
is visible to constraint linking via dict_sys_t::find_table_fk(), so that
two threads loading tables that reference each other can still link the
constraint between them.
Tables that InnoDB internal SQL can refer to are loaded without ever
releasing the latch. The internal SQL parser is not reentrant and is
serialized only by the exclusive dict_sys.latch, and it opens tables in
the middle of parsing; releasing the latch there let another thread run
the parser concurrently and corrupt its state.
How can this PR be tested?
Added
mysql-test/suite/innodb/t/dict_load_concurrent.testto cover the changes.Basing the PR against the correct MariaDB version
mainbranch.PR quality check