MDEV-32176 Contention in ha_innobase::info_low()

During a Sysbench oltp_point_select workload with 1 table and 400
concurrent connections, a bottleneck on dict_table_t::lock_mutex was
observed in ha_innobase::info_low().

dict_table_t::lock_latch: Replaces lock_mutex.

In ha_innobase::info_low() and several other places, we will acquire
a shared dict_table_t::lock_latch or we may elide the latch if
hardware memory transactions are available.

innobase_build_v_templ(): Remove the parameter "bool locked", and
require the caller to hold exclusive dict_table_t::lock_latch
(instead of holding an exclusive dict_sys.latch).

Tested by: Vladislav Vaintroub
Reviewed by: Vladislav Vaintroub

storage/innobase/handler/ha_innodb.cc

@@ -3267,9 +3267,9 @@ static bool innobase_query_caching_table_check_low(
 	}
 #endif
 
-	table->lock_mutex_lock();
+	table->lock_shared_lock();
 	auto len= UT_LIST_GET_LEN(table->locks);
-	table->lock_mutex_unlock();
+	table->lock_shared_unlock();
 	return len == 0;
 }
 
@@ -5596,15 +5596,13 @@ is done when the table first opened.
 @param[in]	ib_table	InnoDB dict_table_t
 @param[in,out]	s_templ		InnoDB template structure
 @param[in]	add_v		new virtual columns added along with
-				add index call
+				add index call */
-@param[in]	locked		true if dict_sys.latch is held */
 void
 innobase_build_v_templ(
 	const TABLE*		table,
 	const dict_table_t*	ib_table,
 	dict_vcol_templ_t*	s_templ,
-	const dict_add_v_col_t*	add_v,
+	const dict_add_v_col_t*	add_v)
-	bool			locked)
 {
 	ulint	ncol = unsigned(ib_table->n_cols) - DATA_N_SYS_COLS;
 	ulint	n_v_col = ib_table->n_v_cols;
@@ -5612,6 +5610,7 @@ innobase_build_v_templ(
 
 	DBUG_ENTER("innobase_build_v_templ");
 	ut_ad(ncol < REC_MAX_N_FIELDS);
+	ut_ad(ib_table->lock_mutex_is_owner());
 
 	if (add_v != NULL) {
 		n_v_col += add_v->n_v_col;
@@ -5619,20 +5618,7 @@ innobase_build_v_templ(
 
 	ut_ad(n_v_col > 0);
 
-	if (!locked) {
-		dict_sys.lock(SRW_LOCK_CALL);
-	}
-
-#if 0
-	/* This does not (need to) hold for ctx->new_table in
-	alter_rebuild_apply_log() */
-	ut_ad(dict_sys.locked());
-#endif
-
 	if (s_templ->vtempl) {
-		if (!locked) {
-			dict_sys.unlock();
-		}
 		DBUG_VOID_RETURN;
 	}
 
@@ -5736,12 +5722,9 @@ innobase_build_v_templ(
 		j++;
 	}
 
-	if (!locked) {
-		dict_sys.unlock();
-	}
-
 	s_templ->db_name = table->s->db.str;
 	s_templ->tb_name = table->s->table_name.str;
+
 	DBUG_VOID_RETURN;
 }
 
@@ -6019,15 +6002,15 @@ ha_innobase::open(const char* name, int, uint)
 	key_used_on_scan = m_primary_key;
 
 	if (ib_table->n_v_cols) {
-		dict_sys.lock(SRW_LOCK_CALL);
+		ib_table->lock_mutex_lock();
+
 		if (ib_table->vc_templ == NULL) {
 			ib_table->vc_templ = UT_NEW_NOKEY(dict_vcol_templ_t());
 			innobase_build_v_templ(
-				table, ib_table, ib_table->vc_templ, NULL,
+				table, ib_table, ib_table->vc_templ);
-				true);
 		}
 
-		dict_sys.unlock();
+		ib_table->lock_mutex_unlock();
 	}
 
 	if (!check_index_consistency(table, ib_table)) {
@@ -14731,7 +14714,7 @@ fsp_get_available_space_in_free_extents(const fil_space_t& space)
 Returns statistics information of the table to the MySQL interpreter,
 in various fields of the handle object.
 @return HA_ERR_* error code or 0 */
-
+TRANSACTIONAL_TARGET
 int
 ha_innobase::info_low(
 /*==================*/
@@ -14812,19 +14795,37 @@ ha_innobase::info_low(
 		ulint	stat_clustered_index_size;
 		ulint	stat_sum_of_other_index_sizes;
 
-		ib_table->stats_mutex_lock();
-
 		ut_a(ib_table->stat_initialized);
 
-		n_rows = ib_table->stat_n_rows;
+#if !defined NO_ELISION && !defined SUX_LOCK_GENERIC
+		if (xbegin()) {
+			if (ib_table->stats_mutex_is_locked())
+				xabort();
 
-		stat_clustered_index_size
+			n_rows = ib_table->stat_n_rows;
-			= ib_table->stat_clustered_index_size;
 
-		stat_sum_of_other_index_sizes
+			stat_clustered_index_size
-			= ib_table->stat_sum_of_other_index_sizes;
+				= ib_table->stat_clustered_index_size;
 
-		ib_table->stats_mutex_unlock();
+			stat_sum_of_other_index_sizes
+				= ib_table->stat_sum_of_other_index_sizes;
+
+			xend();
+		} else
+#endif
+		{
+			ib_table->stats_shared_lock();
+
+			n_rows = ib_table->stat_n_rows;
+
+			stat_clustered_index_size
+				= ib_table->stat_clustered_index_size;
+
+			stat_sum_of_other_index_sizes
+				= ib_table->stat_sum_of_other_index_sizes;
+
+			ib_table->stats_shared_unlock();
+		}
 
 		/*
 		The MySQL optimizer seems to assume in a left join that n_rows
@@ -14944,9 +14945,9 @@ ha_innobase::info_low(
 			stats.create_time = (ulong) stat_info.ctime;
 		}
 
-		ib_table->stats_mutex_lock();
+		ib_table->stats_shared_lock();
 		auto _ = make_scope_exit([ib_table]() {
-			ib_table->stats_mutex_unlock(); });
+			ib_table->stats_shared_unlock(); });
 
 		ut_a(ib_table->stat_initialized);
 

storage/innobase/handler/ha_innodb.h

@@ -880,15 +880,13 @@ innodb_rec_per_key(
 @param[in]	ib_table	InnoDB dict_table_t
 @param[in,out]	s_templ		InnoDB template structure
 @param[in]	add_v		new virtual columns added along with
-				add index call
+				add index call */
-@param[in]	locked		true if innobase_share_mutex is held */
 void
 innobase_build_v_templ(
 	const TABLE*		table,
 	const dict_table_t*	ib_table,
 	dict_vcol_templ_t*	s_templ,
-	const dict_add_v_col_t*	add_v,
+	const dict_add_v_col_t*	add_v = nullptr);
-	bool			locked);
 
 /** callback used by MySQL server layer to initialized
 the table virtual columns' template

storage/innobase/handler/handler0alter.cc

@@ -6521,16 +6521,16 @@ acquire_lock:
 		acquiring an InnoDB table lock even for online operation,
 		to ensure that the rollback of recovered transactions will
 		not run concurrently with online ADD INDEX. */
-		user_table->lock_mutex_lock();
+		user_table->lock_shared_lock();
 		for (lock_t *lock = UT_LIST_GET_FIRST(user_table->locks);
 		     lock;
 		     lock = UT_LIST_GET_NEXT(un_member.tab_lock.locks, lock)) {
 			if (lock->trx->is_recovered) {
-				user_table->lock_mutex_unlock();
+				user_table->lock_shared_unlock();
 				goto acquire_lock;
 			}
 		}
-		user_table->lock_mutex_unlock();
+		user_table->lock_shared_unlock();
 	}
 
 	if (fts_exist) {
@@ -8781,10 +8781,11 @@ ok_exit:
 		}
 		s_templ = UT_NEW_NOKEY(dict_vcol_templ_t());
 
+		ctx->new_table->lock_mutex_lock();
 		innobase_build_v_templ(
-			altered_table, ctx->new_table, s_templ, NULL, false);
+			altered_table, ctx->new_table, s_templ);
-
 		ctx->new_table->vc_templ = s_templ;
+                ctx->new_table->lock_mutex_unlock();
 	} else if (ctx->num_to_add_vcol > 0 && ctx->num_to_drop_vcol == 0) {
 		/* if there is ongoing drop virtual column, then we disallow
 		inplace add index on newly added virtual column, so it does
@@ -8799,10 +8800,12 @@ ok_exit:
 		add_v->v_col = ctx->add_vcol;
 		add_v->v_col_name = ctx->add_vcol_name;
 
+		ctx->new_table->lock_mutex_lock();
 		innobase_build_v_templ(
-			altered_table, ctx->new_table, s_templ, add_v, false);
+			altered_table, ctx->new_table, s_templ, add_v);
 		old_templ = ctx->new_table->vc_templ;
 		ctx->new_table->vc_templ = s_templ;
+		ctx->new_table->lock_mutex_unlock();
 	}
 
 	/* Drop virtual column without rebuild will keep dict table
@@ -11075,11 +11078,10 @@ static bool alter_rebuild_apply_log(
 	if (ctx->new_table->n_v_cols > 0) {
 		s_templ = UT_NEW_NOKEY(
 				dict_vcol_templ_t());
-		s_templ->vtempl = NULL;
+		ctx->new_table->lock_mutex_lock();
-
+		innobase_build_v_templ(altered_table, ctx->new_table, s_templ);
-		innobase_build_v_templ(altered_table, ctx->new_table, s_templ,
-				       NULL, true);
 		ctx->new_table->vc_templ = s_templ;
+		ctx->new_table->lock_mutex_unlock();
 	}
 
 	dberr_t error = row_log_table_apply(

storage/innobase/handler/i_s.cc

@@ -4986,9 +4986,9 @@ i_s_dict_fill_sys_tablestats(THD* thd, dict_table_t *table,
   Field **fields= table_to_fill->field;
 
   {
-    table->stats_mutex_lock();
+    table->stats_shared_lock();
     auto _ = make_scope_exit([table]() {
-      table->stats_mutex_unlock(); dict_sys.unlock(); });
+      table->stats_shared_unlock(); dict_sys.unlock(); });
 
     OK(fields[SYS_TABLESTATS_ID]->store(longlong(table->id), TRUE));
 

storage/innobase/include/dict0dict.h

@@ -700,35 +700,32 @@ bool
 dict_table_has_indexed_v_cols(
 	const dict_table_t*	table);
 
-/********************************************************************//**
+TPOOL_SUPPRESS_TSAN
-Gets the approximately estimated number of rows in the table.
+/** Get the estimated number of rows in the table.
 @return estimated number of rows */
-UNIV_INLINE
+inline uint64_t dict_table_get_n_rows(const dict_table_t *table)
-ib_uint64_t
+{
-dict_table_get_n_rows(
+  ut_ad(table->stat_initialized);
-/*==================*/
+  return table->stat_n_rows;
-	const dict_table_t*	table)	/*!< in: table */
+}
-	MY_ATTRIBUTE((warn_unused_result));
+
-/********************************************************************//**
+/** Increment the number of rows in the table by one.
-Increment the number of rows in the table by one.
+Note that this operation is not protected by any latch,
-Notice that this operation is not protected by any latch, the number is
+the number is approximate. */
-approximate. */
+TPOOL_SUPPRESS_TSAN inline void dict_table_n_rows_inc(dict_table_t *table)
-UNIV_INLINE
+{
-void
+  if (auto n_rows= table->stat_n_rows + 1)
-dict_table_n_rows_inc(
+    table->stat_n_rows= n_rows;
-/*==================*/
+}
-	dict_table_t*	table)	/*!< in/out: table */
+
-	MY_ATTRIBUTE((nonnull));
+/** Decrement the number of rows in the table by one.
-/********************************************************************//**
+Note that this operation is not protected by any latch,
-Decrement the number of rows in the table by one.
+the number is approximate. */
-Notice that this operation is not protected by any latch, the number is
+TPOOL_SUPPRESS_TSAN inline void dict_table_n_rows_dec(dict_table_t *table)
-approximate. */
+{
-UNIV_INLINE
+  if (auto n_rows= table->stat_n_rows)
-void
+    table->stat_n_rows= n_rows - 1;
-dict_table_n_rows_dec(
+}
-/*==================*/
-	dict_table_t*	table)	/*!< in/out: table */
-	MY_ATTRIBUTE((nonnull));
 
 /** Get nth virtual column
 @param[in]	table	target table

storage/innobase/include/dict0dict.inl

@@ -306,56 +306,6 @@ dict_table_has_indexed_v_cols(
 	return(false);
 }
 
-/********************************************************************//**
-Gets the approximately estimated number of rows in the table.
-@return estimated number of rows */
-UNIV_INLINE
-ib_uint64_t
-dict_table_get_n_rows(
-/*==================*/
-	const dict_table_t*	table)	/*!< in: table */
-{
-	ut_ad(table->stat_initialized);
-
-	return(table->stat_n_rows);
-}
-
-/********************************************************************//**
-Increment the number of rows in the table by one.
-Notice that this operation is not protected by any latch, the number is
-approximate. */
-UNIV_INLINE
-void
-dict_table_n_rows_inc(
-/*==================*/
-	dict_table_t*	table)	/*!< in/out: table */
-{
-	if (table->stat_initialized) {
-		ib_uint64_t	n_rows = table->stat_n_rows;
-		if (n_rows < 0xFFFFFFFFFFFFFFFFULL) {
-			table->stat_n_rows = n_rows + 1;
-		}
-	}
-}
-
-/********************************************************************//**
-Decrement the number of rows in the table by one.
-Notice that this operation is not protected by any latch, the number is
-approximate. */
-UNIV_INLINE
-void
-dict_table_n_rows_dec(
-/*==================*/
-	dict_table_t*	table)	/*!< in/out: table */
-{
-	if (table->stat_initialized) {
-		ib_uint64_t	n_rows = table->stat_n_rows;
-		if (n_rows > 0) {
-			table->stat_n_rows = n_rows - 1;
-		}
-	}
-}
-
 #ifdef UNIV_DEBUG
 /********************************************************************//**
 Gets the nth column of a table.

storage/innobase/include/dict0mem.h

@@ -2018,38 +2018,36 @@ struct dict_table_t {
 
 #ifdef UNIV_DEBUG
   /** @return whether the current thread holds the lock_mutex */
-  bool lock_mutex_is_owner() const
+  bool lock_mutex_is_owner() const { return lock_latch.have_wr(); }
-  { return lock_mutex_owner == pthread_self(); }
   /** @return whether the current thread holds the stats_mutex (lock_mutex) */
-  bool stats_mutex_is_owner() const
+  bool stats_mutex_is_owner() const { return lock_latch.have_wr(); }
-  { return lock_mutex_owner == pthread_self(); }
 #endif /* UNIV_DEBUG */
-  void lock_mutex_init() { lock_mutex.init(); }
+  void lock_mutex_init()
-  void lock_mutex_destroy() { lock_mutex.destroy(); }
-  /** Acquire lock_mutex */
-  void lock_mutex_lock()
   {
-    ut_ad(!lock_mutex_is_owner());
+#ifdef UNIV_DEBUG
-    lock_mutex.wr_lock();
+    lock_latch.SRW_LOCK_INIT(0);
-    ut_ad(!lock_mutex_owner.exchange(pthread_self()));
+#else
-  }
+    lock_latch.init();
-  /** Try to acquire lock_mutex */
+#endif
-  bool lock_mutex_trylock()
-  {
-    ut_ad(!lock_mutex_is_owner());
-    bool acquired= lock_mutex.wr_lock_try();
-    ut_ad(!acquired || !lock_mutex_owner.exchange(pthread_self()));
-    return acquired;
-  }
-  /** Release lock_mutex */
-  void lock_mutex_unlock()
-  {
-    ut_ad(lock_mutex_owner.exchange(0) == pthread_self());
-    lock_mutex.wr_unlock();
   }
+  void lock_mutex_destroy() { lock_latch.destroy(); }
+  /** Acquire exclusive lock_latch */
+  void lock_mutex_lock() { lock_latch.wr_lock(ut_d(SRW_LOCK_CALL)); }
+  /** Try to acquire exclusive lock_latch */
+  bool lock_mutex_trylock() { return lock_latch.wr_lock_try(); }
+  /** Release exclusive lock_latch */
+  void lock_mutex_unlock() { lock_latch.wr_unlock(); }
+  /** Acquire shared lock_latch */
+  void lock_shared_lock() { lock_latch.rd_lock(ut_d(SRW_LOCK_CALL)); }
+  /** Release shared lock_latch */
+  void lock_shared_unlock() { lock_latch.rd_unlock(); }
+
 #ifndef SUX_LOCK_GENERIC
-  /** @return whether the lock mutex is held by some thread */
+  /** @return whether an exclusive lock_latch is held by some thread */
-  bool lock_mutex_is_locked() const noexcept { return lock_mutex.is_locked(); }
+  bool lock_mutex_is_locked() const noexcept
+  { return lock_latch.is_write_locked(); }
+  bool stats_mutex_is_locked() const noexcept
+  { return lock_latch.is_write_locked(); }
 #endif
 
   /* stats mutex lock currently defaults to lock_mutex but in the future,
@@ -2060,6 +2058,8 @@ struct dict_table_t {
   void stats_mutex_destroy() { lock_mutex_destroy(); }
   void stats_mutex_lock() { lock_mutex_lock(); }
   void stats_mutex_unlock() { lock_mutex_unlock(); }
+  void stats_shared_lock() { lock_shared_lock(); }
+  void stats_shared_unlock() { lock_shared_unlock(); }
 
   /** Rename the data file.
   @param new_name     name of the table
@@ -2342,18 +2342,19 @@ public:
   /** Mutex protecting autoinc and freed_indexes. */
   srw_spin_mutex autoinc_mutex;
 private:
-  /** Mutex protecting locks on this table. */
-  srw_spin_mutex lock_mutex;
 #ifdef UNIV_DEBUG
-  /** The owner of lock_mutex (0 if none) */
+  typedef srw_lock_debug lock_latch_type;
-  Atomic_relaxed<pthread_t> lock_mutex_owner{0};
+#else
+  typedef srw_spin_lock_low lock_latch_type;
 #endif
+  /** RW-lock protecting locks and statistics on this table */
+  lock_latch_type lock_latch;
 public:
   /** Autoinc counter value to give to the next inserted row. */
   uint64_t autoinc;
 
   /** The transaction that currently holds the the AUTOINC lock on this table.
-  Protected by lock_mutex.
+  Protected by lock_latch.
   The thread that is executing autoinc_trx may read this field without
   holding a latch, in row_lock_table_autoinc_for_mysql().
   Only the autoinc_trx thread may clear this field; it cannot be
@@ -2412,9 +2413,9 @@ public:
 	/** Magic number. */
 	ulint					magic_n;
 #endif /* UNIV_DEBUG */
-	/** mysql_row_templ_t for base columns used for compute the virtual
+  /** mysql_row_templ_t for base columns used for compute the virtual
-	columns */
+  columns; protected by lock_latch */
-	dict_vcol_templ_t*			vc_templ;
+  dict_vcol_templ_t *vc_templ;
 
   /* @return whether the table has any other transcation lock
   other than the given transaction */

storage/innobase/lock/lock0lock.cc

@@ -6388,7 +6388,7 @@ resolve_table_lock:
       if (!table->lock_mutex_trylock())
       {
         /* The correct latching order is:
-        lock_sys.latch, table->lock_mutex_lock(), lock_sys.wait_mutex.
+        lock_sys.latch, table->lock_latch, lock_sys.wait_mutex.
         Thus, we must release lock_sys.wait_mutex for a blocking wait. */
         mysql_mutex_unlock(&lock_sys.wait_mutex);
         table->lock_mutex_lock();
@@ -6585,9 +6585,9 @@ bool lock_table_has_locks(dict_table_t *table)
   else
 #endif
   {
-    table->lock_mutex_lock();
+    table->lock_shared_lock();
     len= UT_LIST_GET_LEN(table->locks);
-    table->lock_mutex_unlock();
+    table->lock_shared_unlock();
   }
   if (len)
     return true;