Revert "MDEV-32899 InnoDB is holding shared dict_sys.latch while waiting for...

Revert "MDEV-32899 InnoDB is holding shared dict_sys.latch while waiting for FOREIGN KEY child table lock on DDL"

This reverts commit 569da6a7,
commit 768a7361, and
commit ba6bf7ad
because of a regression that was filed as MDEV-33104.

mysql-test/suite/perfschema/r/sxlock_func,debug.rdiff

-@@ -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;

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';
 

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);
 

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);
 		}

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);

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<ulonglong> latch_ex_wait_start;
 
+  /** the rw-latch protecting the data dictionary cache */
+  alignas(CPU_LEVEL1_DCACHE_LINESIZE) srw_lock latch;
 #ifdef UNIV_DEBUG
-  typedef index_lock dict_lock;
-#else
-  typedef srw_lock dict_lock;
+  /** whether latch is being held in exclusive mode (by any thread) */
+  Atomic_relaxed<pthread_t> latch_ex;
+  /** number of S-latch holders */
+  Atomic_counter<uint32_t> latch_readers;
 #endif
-
-  /** the rw-latch protecting the data dictionary cache */
-  alignas(CPU_LEVEL1_DCACHE_LINESIZE) dict_lock latch;
 public:
   /** Indexes of SYS_TABLE[] */
   enum
@@ -1491,12 +1491,15 @@ class dict_sys_t
   }
 
 #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 @@ class dict_sys_t
   /** 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 @@ class dict_sys_t
   /** 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
 

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

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 @@ dberr_t lock_table_for_trx(dict_table_t *table, trx_t *trx, lock_mode mode,
   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<dict_table_t*> 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