BUG#22083048: MUTEX REQUEST ON LOCK_OPEN CAUSE HANG IN OPEN_TABLE_UNCACHED()

Problem: Adding a virtual generated column could cause a deadlock in
debug builds. This was caused by a virtual column purge thread
callback becoming blocked in open_table_uncached() because it could
not acquire the LOCK_open mutex. The mutex, in turn, was needed to
check if the table being opened was present in the TDC.

Solution: Relax condition for assert and associated lock so that it is
possible to call open_table_uncached() without taking LOCK_open or
checking the TDC provided the table will not be opened in SE. Relaxing
the assert is safe in this case as the second share outside the TDC
will not have its own SE private data.

Author: Dyre Tjeldvoll

sql/handler.cc

@@ -8193,6 +8193,11 @@ bool handler::my_prepare_gcolumn_template(THD *thd,
   DBUG_ASSERT(!was_truncated);
   lex_start(thd);
   bool rc= true;
+
+  // Note! The last argument to open_table_uncached() must be false,
+  // since the table already exists in the TDC. Allowing the table to
+  // be opened in the SE in this case is dangerous as the two shares
+  // could get conflicting SE private data.
   TABLE *table= open_table_uncached(thd, path, db_name, table_name,
                                     false, false);
   if (table)
@@ -8207,14 +8212,17 @@ bool handler::my_prepare_gcolumn_template(THD *thd,
 
 
 /**
-   Callback for generated columns processing. Will open the table
-   and call my_eval_gcolumn_expr_helper() to do the actual
+   Callback for generated columns processing. Will open the table, in the
+   server *only*, and call my_eval_gcolumn_expr_helper() to do the actual
    processing. This function is a variant of the other
    handler::my_eval_gcolumn_expr() but is intended for use when no TABLE
    object already exists - e.g. from purge threads.
 
+   Note! The call to open_table_uncached() must be made with the last
+   argument (open_in_engine) set to false. Failing to do so will cause
+   deadlocks and incorrect behavior.
+
    @param thd             Thread handle
-   @param open_in_engine  Should ha_open() be called?
    @param db_name         Database containing the table to open
    @param table_name      Name of table to open
    @param fields          Bitmap of field index of evaluated generated column
@@ -8223,49 +8231,28 @@ bool handler::my_prepare_gcolumn_template(THD *thd,
    @return true in case of error, false otherwise.
 */
 
-bool handler::my_eval_gcolumn_expr(THD *thd,
-                                   bool open_in_engine,
-                                   const char *db_name,
-                                   const char *table_name,
-                                   const MY_BITMAP *const fields,
-                                   uchar *record)
+bool handler::my_eval_gcolumn_expr_with_open(THD *thd,
+                                             const char *db_name,
+                                             const char *table_name,
+                                             const MY_BITMAP *const fields,
+                                             uchar *record)
 {
   DBUG_ASSERT(thd->system_thread == SYSTEM_THREAD_BACKGROUND);
   bool retval= true;
   lex_start(thd);
 
-  if (open_in_engine)
-  {
-    TABLE_LIST table_list;
-    table_list.init_one_table(db_name, strlen(db_name),
-                              table_name, strlen(table_name),
-                              table_name, TL_READ);
+  char path[FN_REFLEN + 1];
+  bool was_truncated;
+  build_table_filename(path, sizeof(path) - 1 - reg_ext_length,
+                       db_name, table_name, "", 0, &was_truncated);
+  DBUG_ASSERT(!was_truncated);
 
-    TABLE *table= open_ltable(thd, &table_list, table_list.lock_type,
-                              MYSQL_LOCK_IGNORE_TIMEOUT);
-    if (table)
-    {
-      retval=
-        my_eval_gcolumn_expr_helper(thd, table_list.table, fields, record, true);
-      trans_commit_stmt(thd);
-      close_thread_tables(thd);
-    }
-  }
-  else
+  TABLE *table= open_table_uncached(thd, path, db_name, table_name,
+                                    false, false);
+  if (table)
   {
-    char path[FN_REFLEN + 1];
-    bool was_truncated;
-    build_table_filename(path, sizeof(path) - 1 - reg_ext_length,
-                         db_name, table_name, "", 0, &was_truncated);
-    DBUG_ASSERT(!was_truncated);
-
-    TABLE *table= open_table_uncached(thd, path, db_name, table_name,
-                                      false, false);
-    if (table)
-    {
-      retval= my_eval_gcolumn_expr_helper(thd, table, fields, record, true);
-      intern_close_table(table);
-    }
+    retval= my_eval_gcolumn_expr_helper(thd, table, fields, record, true);
+    intern_close_table(table);
   }
 
   lex_end(thd->lex);

sql/handler.h

@@ -3693,12 +3693,11 @@ class handler :public Sql_alloc
                                           const char *table_name,
                                           my_gcolumn_template_callback_t myc,
                                           void *ib_table);
-  static bool my_eval_gcolumn_expr(THD *thd,
-                                   bool open_in_engine,
-                                   const char *db_name,
-                                   const char *table_name,
-                                   const MY_BITMAP *const fields,
-                                   uchar *record);
+  static bool my_eval_gcolumn_expr_with_open(THD *thd,
+                                             const char *db_name,
+                                             const char *table_name,
+                                             const MY_BITMAP *const fields,
+                                             uchar *record);
   static bool my_eval_gcolumn_expr(THD *thd,
                                    const char *db_name,
                                    const char *table_name,

sql/sql_base.cc

@@ -6779,10 +6779,25 @@ TABLE *open_table_uncached(THD *thd, const char *path, const char *db,
     DBUG_RETURN(0);				/* purecov: inspected */
 
 #ifndef DBUG_OFF
-  mysql_mutex_lock(&LOCK_open);
-  DBUG_ASSERT(!my_hash_search(&table_def_cache, (uchar*) cache_key,
-                              key_length));
-  mysql_mutex_unlock(&LOCK_open);
+  // In order to let purge thread callback call open_table_uncached()
+  // we cannot grab LOCK_open here, as that will cause a deadlock.
+
+  // The assert below safeguards against opening a table which is
+  // already found in the table definition cache. Iff the table will
+  // be opened in the SE below, we may get two conflicting copies of
+  // SE private data in the two table_shares.
+
+  // By only grabbing LOCK_open and check the assert only when
+  // open_in_engine is true, we safeguard the engine private data while
+  // also allowing the purge threads callbacks since they always call
+  // with open_in_engine=false.
+  if (open_in_engine)
+  {
+    mysql_mutex_lock(&LOCK_open);
+    DBUG_ASSERT(!my_hash_search(&table_def_cache, (uchar*) cache_key,
+                                key_length));
+    mysql_mutex_unlock(&LOCK_open);
+  }
 #endif
 
   share= (TABLE_SHARE*) (tmp_table+1);

storage/innobase/handler/ha_innodb.cc

@@ -19932,9 +19932,8 @@ innobase_get_computed_value(
 				vctempl->mysql_col_len, blob_mem, max_len);
                 }
 
-		ret = handler::my_eval_gcolumn_expr(
-			current_thd, false,
-			index->table->vc_templ->db_name.c_str(),
+		ret = handler::my_eval_gcolumn_expr_with_open(
+			current_thd, index->table->vc_templ->db_name.c_str(),
 			index->table->vc_templ->tb_name.c_str(), &column_map,
 			(uchar *)mysql_rec);
         } else {