From 21560bee9d6351b6f934a889bed123b548a49bf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Fri, 19 Jan 2024 12:46:11 +0200 Subject: [PATCH] Revert "MDEV-32899 InnoDB is holding shared dict_sys.latch while waiting for FOREIGN KEY child table lock on DDL" This reverts commit 569da6a7bab034fc9768af88d8dbc2a8e2944764, commit 768a736174d6caf09df43e84b0c1b9ec52f1a301, and commit ba6bf7ad9e52c1dc31a22a619c17e1bb55b46d5d because of a regression that was filed as MDEV-33104. --- .../perfschema/r/sxlock_func,debug.rdiff | 22 ------- .../suite/perfschema/t/sxlock_func.test | 1 - storage/innobase/dict/dict0dict.cc | 37 ++++-------- storage/innobase/handler/ha_innodb.cc | 37 ++++++++---- storage/innobase/handler/handler0alter.cc | 11 +++- storage/innobase/include/dict0dict.h | 60 +++++++++---------- storage/innobase/include/lock0lock.h | 7 --- storage/innobase/lock/lock0lock.cc | 32 ---------- 8 files changed, 79 insertions(+), 128 deletions(-) delete mode 100644 mysql-test/suite/perfschema/r/sxlock_func,debug.rdiff diff --git a/mysql-test/suite/perfschema/r/sxlock_func,debug.rdiff b/mysql-test/suite/perfschema/r/sxlock_func,debug.rdiff deleted file mode 100644 index 0596810e553..00000000000 --- a/mysql-test/suite/perfschema/r/sxlock_func,debug.rdiff +++ /dev/null @@ -1,22 +0,0 @@ -@@ -7,7 +7,6 @@ - WHERE name LIKE 'wait/synch/rwlock/innodb/%' - AND name!='wait/synch/rwlock/innodb/btr_search_latch' ORDER BY name; - name --wait/synch/rwlock/innodb/dict_operation_lock - wait/synch/rwlock/innodb/fil_space_latch - wait/synch/rwlock/innodb/lock_latch - wait/synch/rwlock/innodb/trx_i_s_cache_lock -@@ -19,11 +18,13 @@ - select name from performance_schema.setup_instruments - where name like "wait/synch/sxlock/%" order by name; - name -+wait/synch/sxlock/innodb/dict_operation_lock - wait/synch/sxlock/innodb/index_tree_rw_lock - SELECT DISTINCT name FROM performance_schema.rwlock_instances - WHERE name LIKE 'wait/synch/sxlock/innodb/%' - ORDER BY name; - name -+wait/synch/sxlock/innodb/dict_operation_lock - wait/synch/sxlock/innodb/index_tree_rw_lock - create table t1(a int) engine=innodb; - begin; diff --git a/mysql-test/suite/perfschema/t/sxlock_func.test b/mysql-test/suite/perfschema/t/sxlock_func.test index 24d0e07ca41..c43adc849d2 100644 --- a/mysql-test/suite/perfschema/t/sxlock_func.test +++ b/mysql-test/suite/perfschema/t/sxlock_func.test @@ -5,7 +5,6 @@ --source include/not_embedded.inc --source include/have_perfschema.inc --source include/have_innodb.inc ---source include/maybe_debug.inc UPDATE performance_schema.setup_instruments SET enabled = 'NO', timed = 'YES'; diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/dict0dict.cc index d51ced003e8..a66bd0df510 100644 --- a/storage/innobase/dict/dict0dict.cc +++ b/storage/innobase/dict/dict0dict.cc @@ -958,12 +958,11 @@ void dict_sys_t::lock_wait(SRW_LOCK_ARGS(const char *file, unsigned line)) if (latch_ex_wait_start.compare_exchange_strong (old, now, std::memory_order_relaxed, std::memory_order_relaxed)) { -#ifdef UNIV_DEBUG - latch.x_lock(SRW_LOCK_ARGS(file, line)); -#else latch.wr_lock(SRW_LOCK_ARGS(file, line)); -#endif latch_ex_wait_start.store(0, std::memory_order_relaxed); + ut_ad(!latch_readers); + ut_ad(!latch_ex); + ut_d(latch_ex= pthread_self()); return; } @@ -978,39 +977,33 @@ void dict_sys_t::lock_wait(SRW_LOCK_ARGS(const char *file, unsigned line)) if (waited > threshold / 4) ib::warn() << "A long wait (" << waited << " seconds) was observed for dict_sys.latch"; -#ifdef UNIV_DEBUG - latch.x_lock(SRW_LOCK_ARGS(file, line)); -#else latch.wr_lock(SRW_LOCK_ARGS(file, line)); -#endif + ut_ad(!latch_readers); + ut_ad(!latch_ex); + ut_d(latch_ex= pthread_self()); } #ifdef UNIV_PFS_RWLOCK ATTRIBUTE_NOINLINE void dict_sys_t::unlock() { -# ifdef UNIV_DEBUG - latch.x_unlock(); -# else + ut_ad(latch_ex == pthread_self()); + ut_ad(!latch_readers); + ut_d(latch_ex= 0); latch.wr_unlock(); -# endif } ATTRIBUTE_NOINLINE void dict_sys_t::freeze(const char *file, unsigned line) { -# ifdef UNIV_DEBUG - latch.s_lock(file, line); -# else latch.rd_lock(file, line); -# endif + ut_ad(!latch_ex); + ut_d(latch_readers++); } ATTRIBUTE_NOINLINE void dict_sys_t::unfreeze() { -# ifdef UNIV_DEBUG - latch.s_unlock(); -# else + ut_ad(!latch_ex); + ut_ad(latch_readers--); latch.rd_unlock(); -# endif } #endif /* UNIV_PFS_RWLOCK */ @@ -4544,11 +4537,7 @@ void dict_sys_t::close() temp_id_hash.free(); unlock(); -#ifdef UNIV_DEBUG - latch.free(); -#else latch.destroy(); -#endif mysql_mutex_destroy(&dict_foreign_err_mutex); diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 5436956479a..98f3fa0344d 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -598,13 +598,7 @@ static PSI_rwlock_info all_innodb_rwlocks[] = # ifdef BTR_CUR_HASH_ADAPT { &btr_search_latch_key, "btr_search_latch", 0 }, # endif - { &dict_operation_lock_key, "dict_operation_lock", -# ifdef UNIV_DEBUG - PSI_RWLOCK_FLAG_SX -# else - 0 -# endif - }, + { &dict_operation_lock_key, "dict_operation_lock", 0 }, { &fil_space_latch_key, "fil_space_latch", 0 }, { &trx_i_s_cache_lock_key, "trx_i_s_cache_lock", 0 }, { &trx_purge_latch_key, "trx_purge_latch", 0 }, @@ -13546,7 +13540,14 @@ int ha_innobase::delete_table(const char *name) /* FOREIGN KEY constraints cannot exist on partitioned tables. */; #endif else - err= lock_table_children(table, trx); + { + dict_sys.freeze(SRW_LOCK_CALL); + for (const dict_foreign_t* f : table->referenced_set) + if (dict_table_t* child= f->foreign_table) + if ((err= lock_table_for_trx(child, trx, LOCK_X)) != DB_SUCCESS) + break; + dict_sys.unfreeze(); + } } dict_table_t *table_stats= nullptr, *index_stats= nullptr; @@ -13944,7 +13945,14 @@ int ha_innobase::truncate() dict_table_t *table_stats = nullptr, *index_stats = nullptr; MDL_ticket *mdl_table = nullptr, *mdl_index = nullptr; - dberr_t error= lock_table_children(ib_table, trx); + dberr_t error= DB_SUCCESS; + + dict_sys.freeze(SRW_LOCK_CALL); + for (const dict_foreign_t *f : ib_table->referenced_set) + if (dict_table_t *child= f->foreign_table) + if ((error= lock_table_for_trx(child, trx, LOCK_X)) != DB_SUCCESS) + break; + dict_sys.unfreeze(); if (error == DB_SUCCESS) error= lock_table_for_trx(ib_table, trx, LOCK_X); @@ -14135,7 +14143,16 @@ ha_innobase::rename_table( /* There is no need to lock any FOREIGN KEY child tables. */ } else if (dict_table_t *table = dict_table_open_on_name( norm_from, false, DICT_ERR_IGNORE_FK_NOKEY)) { - error = lock_table_children(table, trx); + dict_sys.freeze(SRW_LOCK_CALL); + for (const dict_foreign_t* f : table->referenced_set) { + if (dict_table_t* child = f->foreign_table) { + error = lock_table_for_trx(child, trx, LOCK_X); + if (error != DB_SUCCESS) { + break; + } + } + } + dict_sys.unfreeze(); if (error == DB_SUCCESS) { error = lock_table_for_trx(table, trx, LOCK_X); } diff --git a/storage/innobase/handler/handler0alter.cc b/storage/innobase/handler/handler0alter.cc index dc5c1f71925..3b892434a67 100644 --- a/storage/innobase/handler/handler0alter.cc +++ b/storage/innobase/handler/handler0alter.cc @@ -11203,7 +11203,16 @@ ha_innobase::commit_inplace_alter_table( fts_optimize_remove_table(ctx->old_table); } - error = lock_table_children(ctx->old_table, trx); + dict_sys.freeze(SRW_LOCK_CALL); + for (auto f : ctx->old_table->referenced_set) { + if (dict_table_t* child = f->foreign_table) { + error = lock_table_for_trx(child, trx, LOCK_X); + if (error != DB_SUCCESS) { + break; + } + } + } + dict_sys.unfreeze(); if (ctx->new_table->fts) { ut_ad(!ctx->new_table->fts->add_wq); diff --git a/storage/innobase/include/dict0dict.h b/storage/innobase/include/dict0dict.h index e4fc58007cf..895743be84b 100644 --- a/storage/innobase/include/dict0dict.h +++ b/storage/innobase/include/dict0dict.h @@ -1316,14 +1316,14 @@ class dict_sys_t /** The my_hrtime_coarse().val of the oldest lock_wait() start, or 0 */ std::atomic latch_ex_wait_start; -#ifdef UNIV_DEBUG - typedef index_lock dict_lock; -#else - typedef srw_lock dict_lock; -#endif - /** the rw-latch protecting the data dictionary cache */ - alignas(CPU_LEVEL1_DCACHE_LINESIZE) dict_lock latch; + alignas(CPU_LEVEL1_DCACHE_LINESIZE) srw_lock latch; +#ifdef UNIV_DEBUG + /** whether latch is being held in exclusive mode (by any thread) */ + Atomic_relaxed latch_ex; + /** number of S-latch holders */ + Atomic_counter latch_readers; +#endif public: /** Indexes of SYS_TABLE[] */ enum @@ -1491,12 +1491,15 @@ public: } #ifdef UNIV_DEBUG - /** @return whether the current thread is holding the latch */ - bool frozen() const { return latch.have_any(); } - /** @return whether the current thread is holding a shared latch */ - bool frozen_not_locked() const { return latch.have_s(); } + /** @return whether any thread (not necessarily the current thread) + is holding the latch; that is, this check may return false + positives */ + bool frozen() const { return latch_readers || latch_ex; } + /** @return whether any thread (not necessarily the current thread) + is holding a shared latch */ + bool frozen_not_locked() const { return latch_readers; } /** @return whether the current thread holds the exclusive latch */ - bool locked() const { return latch.have_x(); } + bool locked() const { return latch_ex == pthread_self(); } #endif private: /** Acquire the exclusive latch */ @@ -1511,12 +1514,13 @@ public: /** Exclusively lock the dictionary cache. */ void lock(SRW_LOCK_ARGS(const char *file, unsigned line)) { -#ifdef UNIV_DEBUG - ut_ad(!latch.have_any()); - if (!latch.x_lock_try()) -#else - if (!latch.wr_lock_try()) -#endif + if (latch.wr_lock_try()) + { + ut_ad(!latch_readers); + ut_ad(!latch_ex); + ut_d(latch_ex= pthread_self()); + } + else lock_wait(SRW_LOCK_ARGS(file, line)); } @@ -1531,30 +1535,24 @@ public: /** Unlock the data dictionary cache. */ void unlock() { -# ifdef UNIV_DEBUG - latch.x_unlock(); -# else + ut_ad(latch_ex == pthread_self()); + ut_ad(!latch_readers); + ut_d(latch_ex= 0); latch.wr_unlock(); -# endif } /** Acquire a shared lock on the dictionary cache. */ void freeze() { -# ifdef UNIV_DEBUG - ut_ad(!latch.have_any()); - latch.s_lock(); -# else latch.rd_lock(); -# endif + ut_ad(!latch_ex); + ut_d(latch_readers++); } /** Release a shared lock on the dictionary cache. */ void unfreeze() { -# ifdef UNIV_DEBUG - latch.s_unlock(); -# else + ut_ad(!latch_ex); + ut_ad(latch_readers--); latch.rd_unlock(); -# endif } #endif diff --git a/storage/innobase/include/lock0lock.h b/storage/innobase/include/lock0lock.h index 65537859924..59ee7f551b4 100644 --- a/storage/innobase/include/lock0lock.h +++ b/storage/innobase/include/lock0lock.h @@ -438,13 +438,6 @@ dberr_t lock_table_for_trx(dict_table_t *table, trx_t *trx, lock_mode mode, bool no_wait= false) MY_ATTRIBUTE((nonnull, warn_unused_result)); -/** Lock the child tables of a table. -@param table parent table -@param trx transaction -@return error code */ -dberr_t lock_table_children(dict_table_t *table, trx_t *trx) - MY_ATTRIBUTE((nonnull, warn_unused_result)); - /** Exclusively lock the data dictionary tables. @param trx dictionary transaction @return error code diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc index c9072998e66..df51ceb16d8 100644 --- a/storage/innobase/lock/lock0lock.cc +++ b/storage/innobase/lock/lock0lock.cc @@ -3940,8 +3940,6 @@ static void lock_table_dequeue(lock_t *in_lock, bool owns_wait_mutex) dberr_t lock_table_for_trx(dict_table_t *table, trx_t *trx, lock_mode mode, bool no_wait) { - ut_ad(!dict_sys.frozen()); - mem_heap_t *heap= mem_heap_create(512); sel_node_t *node= sel_node_create(heap); que_thr_t *thr= pars_complete_graph_for_exec(node, trx, heap, nullptr); @@ -3978,36 +3976,6 @@ run_again: return err; } -/** Lock the child tables of a table. -@param table parent table -@param trx transaction -@return error code */ -dberr_t lock_table_children(dict_table_t *table, trx_t *trx) -{ - dict_sys.freeze(SRW_LOCK_CALL); - std::vector children; - - for (auto f : table->referenced_set) - if (dict_table_t *child= f->foreign_table) - { - child->acquire(); - children.emplace_back(child); - } - dict_sys.unfreeze(); - - dberr_t err= DB_SUCCESS; - - for (auto child : children) - if ((err= lock_table_for_trx(child, trx, LOCK_X)) != DB_SUCCESS) - break; - - for (auto child : children) - child->release(); - - return err; -} - - /** Exclusively lock the data dictionary tables. @param trx dictionary transaction @return error code