Skip to content

MDEV-35154 : dict_sys_t::load_table() is holding exclusive dict_sys.latch for unnecessarily long time - #5606

Open
mariadb-TafzeelShams wants to merge 1 commit into
11.4from
10.11-MDEV-35154
Open

mariadb-TafzeelShams wants to merge 1 commit into
11.4from
10.11-MDEV-35154

Conversation

@mariadb-TafzeelShams

@mariadb-TafzeelShams mariadb-TafzeelShams commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor
  • The Jira issue number for this PR is: MDEV-35154

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.test to cover the changes.

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

@mariadb-TafzeelShams
mariadb-TafzeelShams marked this pull request as ready for review August 27, 2026 13:51

@iMineLink iMineLink left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I think the core mechanism is sound and worth merging after checking my minor comments, and getting @dr-m review.

Comment thread storage/innobase/row/row0mysql.cc Outdated
/* 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if heap could be reused instead.

Comment thread storage/innobase/include/dict0mem.h Outdated
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread storage/innobase/include/dict0dict.inl Outdated
{
/* A table whose definition is still being loaded must never be
referenced; dict_sys_t::find_table() hides such tables. */
ut_ad(!loading);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this relies on the "initial state" of the loading field being not 0 == dict_table_t::NOT_LOADING.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread storage/innobase/dict/dict0dict.cc Outdated
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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread storage/innobase/dict/dict0dict.cc Outdated
/* 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this dict_load_hold_latch() has slipped between dict_load_table_one() below and its docstring above.

Comment thread storage/innobase/dict/dict0load.cc Outdated

const char* n = static_cast<const char*>(p) + 1;

if (name.end() - n > 4

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe name.end() > n + 4 is safer?

Comment thread storage/innobase/dict/dict0load.cc Outdated
err_exit:
mtr.commit();
if (uncommitted) {
/* A stub was removed at reload:; wake up any

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra : in comment?

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread storage/innobase/handler/ha_innodb.cc Outdated
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);
Comment thread storage/innobase/dict/dict0dict.cc Outdated
Comment on lines +970 to +972
latch.SRW_LOCK_INIT(dict_operation_lock_key);
mysql_mutex_init(dict_load_mutex_key, &load_mutex, nullptr);
pthread_cond_init(&load_cond, nullptr);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Comment thread storage/innobase/include/dict0mem.h Outdated
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread storage/innobase/row/row0mysql.cc Outdated
Comment on lines +2554 to +2555
mem_heap_t* heap = NULL;
mem_heap_t* fk_heap = NULL;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use heap for both uses.

Comment thread storage/innobase/include/dict0dict.inl Outdated
{
/* A table whose definition is still being loaded must never be
referenced; dict_sys_t::find_table() hides such tables. */
ut_ad(!loading);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@CLAassistant

CLAassistant commented Sep 18, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@mariadb-TafzeelShams
mariadb-TafzeelShams changed the base branch from 10.11 to 11.4 September 18, 2026 11:50
@mariadb-TafzeelShams
mariadb-TafzeelShams force-pushed the 10.11-MDEV-35154 branch 2 times, most recently from 8ba7981 to 698901b Compare September 23, 2026 01:30

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an unintentional change of the submodule. Beware of git commit -a.

Comment on lines +248 to +254
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread storage/innobase/dict/dict0dict.cc Outdated
Comment on lines +606 to +609
/* A table that is still being loaded is treated as if it
were not cached. */
if (t->loading())
return false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread storage/innobase/dict/dict0dict.cc Outdated
Comment on lines +628 to +635
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +642 to +651
/* 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike the changes to dict_sys_t::find_table(), this one may be unavoidable.

Comment on lines +2340 to +2347
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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +2628 to +2630
evict:
dict_load_table_one_discard(table);
mem_heap_free(heap);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread storage/innobase/dict/dict0load.cc Outdated
Comment on lines +2696 to +2700
/* 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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why retry the lookup? wait_for_load() should return a final status.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread storage/innobase/include/dict0dict.inl Outdated
Comment on lines +1089 to +1091
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified to return n == 1;

Comment thread storage/innobase/include/dict0mem.h Outdated
Comment on lines +1929 to +1932
/** 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; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@mariadb-TafzeelShams mariadb-TafzeelShams Oct 20, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mariadb-TafzeelShams
mariadb-TafzeelShams marked this pull request as draft October 6, 2026 11:55
@mariadb-TafzeelShams
mariadb-TafzeelShams marked this pull request as ready for review October 12, 2026 02:24
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants