Bug#21824519: ASSERTION IN DROP TRIGGER WHEN TABLE HAS VIRTUAL GENERATED COLUMN

In some cases a DROP TRIGGER statement could hit a DBUG_ASSERT in
debug builds if the trigger was defined on a table that contained a
generated column. The same DBUG_ASSERT could under some circumstances
be hit when calling a stored function that referenced a table that
contained a generated column.

The DBUG_ASSERT was hit during re-resolving (aka "refixing") of the
generated column expression. The first time a statement opens a table
that contains a generated column, all the generated columns in that
table will be re-resolved to ensure that they are in a pristine state.
The DBUG_ASSERT (in TABLE_LIST::map()) fails because the TABLE_LIST
object associated with the Item_field that is being re-resolved, is in
an inconsistent state where m_map is not in sync with m_tableno.

The TABLE_LIST objects in question are allocated in
sp_add_to_query_tables() in the DROP TRIGGER case and in
sp_head::add_used_tables_to_table_list() in the stored function case.
Common for these two functions is that they initialize the TABLE_LIST
objects by manually setting the various members, instead of calling
TABLE_LIST::init_one_table(), which is the common way of initializing
such objects. This manual initialization forgets to set the m_map and
m_tableno members to values that are consistent with each other.

The fix makes the functions use init_one_table() to initialize the
TABLE_LIST objects. init_one_table() also needed a little reorganizing
to allow more flexible initialization of the associated lock request.

(cherry picked from commit 1aabcfde317a5b96bf9d33226795aa096db2aab8)

Author: kahatlen

mysql-test/suite/gcol/r/gcol_bugfixes.result

@@ -443,3 +443,26 @@ SELECT MAX(a) FROM v WHERE b=1;
 MAX(a)
 1
 DROP TABLE v;
+#
+# Bug#21824519: ASSERTION IN DROP TRIGGER WHEN TABLE HAS
+#               VIRTUAL GENERATED COLUMN
+#
+CREATE TABLE t (a INT, b INT GENERATED ALWAYS AS (a) VIRTUAL);
+CREATE TRIGGER tr BEFORE INSERT ON t FOR EACH ROW BEGIN END;
+INSERT INTO t (a) VALUES (1);
+SELECT * FROM t;
+a	b
+1	1
+DROP TRIGGER tr;
+SELECT * FROM t;
+a	b
+1	1
+CREATE FUNCTION f() RETURNS INT RETURN (SELECT COUNT(*) FROM t);
+SELECT f();
+f()
+1
+DROP FUNCTION f;
+SELECT * FROM t;
+a	b
+1	1
+DROP TABLE t;

mysql-test/suite/gcol/t/gcol_bugfixes.test

@@ -443,3 +443,21 @@ b INT, KEY(b));
 INSERT INTO v (a,b) VALUES (1,1);
 SELECT MAX(a) FROM v WHERE b=1;
 DROP TABLE v;
+
+--echo #
+--echo # Bug#21824519: ASSERTION IN DROP TRIGGER WHEN TABLE HAS
+--echo #               VIRTUAL GENERATED COLUMN
+--echo #
+CREATE TABLE t (a INT, b INT GENERATED ALWAYS AS (a) VIRTUAL);
+CREATE TRIGGER tr BEFORE INSERT ON t FOR EACH ROW BEGIN END;
+INSERT INTO t (a) VALUES (1);
+SELECT * FROM t;
+# DROP TRIGGER used to hit a DBUG_ASSERT.
+DROP TRIGGER tr;
+SELECT * FROM t;
+CREATE FUNCTION f() RETURNS INT RETURN (SELECT COUNT(*) FROM t);
+# And this function call hit the same DBUG_ASSERT.
+SELECT f();
+DROP FUNCTION f;
+SELECT * FROM t;
+DROP TABLE t;

sql/sp.cc

@@ -2626,26 +2626,24 @@ bool sp_check_name(LEX_STRING *ident)
   the global table list, e.g. "mysql", "proc".
 */
 TABLE_LIST *sp_add_to_query_tables(THD *thd, LEX *lex,
-                                   const char *db, const char *name,
-                                   thr_lock_type locktype,
-                                   enum_mdl_type mdl_type)
+                                   const char *db, const char *name)
 {
-  TABLE_LIST *table= (TABLE_LIST *)thd->mem_calloc(sizeof(TABLE_LIST));
+  TABLE_LIST *table= static_cast<TABLE_LIST*>(thd->alloc(sizeof(TABLE_LIST)));
 
   if (!table)
     return NULL;
 
-  table->db_length= strlen(db);
-  table->db= thd->strmake(db, table->db_length);
-  table->table_name_length= strlen(name);
-  table->table_name= thd->strmake(name, table->table_name_length);
-  table->alias= thd->mem_strdup(name);
-  table->lock_type= locktype;
+  size_t db_length= strlen(db);
+  size_t table_name_length= strlen(name);
+
+  table->init_one_table(thd->strmake(db, db_length), db_length,
+                        thd->strmake(name, table_name_length),
+                        table_name_length,
+                        thd->mem_strdup(name),
+                        TL_IGNORE, MDL_SHARED_NO_WRITE);
+
   table->select_lex= lex->current_select();
   table->cacheable_table= 1;
-  MDL_REQUEST_INIT(&table->mdl_request,
-                   MDL_key::TABLE, table->db, table->table_name,
-                   mdl_type, MDL_TRANSACTION);
 
   lex->add_to_query_tables(table);
 

sql/sp.h

@@ -207,9 +207,7 @@ uint sp_get_flags_for_command(LEX *lex);
 bool sp_check_name(LEX_STRING *ident);
 
 TABLE_LIST *sp_add_to_query_tables(THD *thd, LEX *lex,
-                                   const char *db, const char *name,
-                                   thr_lock_type locktype,
-                                   enum_mdl_type mdl_type);
+                                   const char *db, const char *name);
 
 bool sp_check_show_access(THD *thd, sp_head *sp, bool *full_access);
 

sql/sp_head.cc

@@ -32,6 +32,7 @@
 #include "sp_cache.h"
 #include "sql_parse.h"         // cleanup_items
 #include "sql_base.h"          // close_thread_tables
+#include "template_utils.h"    // pointer_cast
 #include "transaction.h"       // trans_commit_stmt
 #include "opt_trace.h"         // opt_trace_disable_etc
 
@@ -2101,31 +2102,19 @@ void sp_head::add_used_tables_to_table_list(THD *thd,
 
   for (uint i= 0; i < m_sptabs.records; i++)
   {
-    char *tab_buff, *key_buff;
-    SP_TABLE *stab= (SP_TABLE*) my_hash_element(&m_sptabs, i);
+    SP_TABLE *stab= pointer_cast<SP_TABLE*>(my_hash_element(&m_sptabs, i));
     if (stab->temp)
       continue;
 
-    if (!(tab_buff= (char *)thd->mem_calloc(ALIGN_SIZE(sizeof(TABLE_LIST)) *
-                                        stab->lock_count)) ||
-        !(key_buff= (char*)thd->memdup(stab->qname.str,
-                                       stab->qname.length)))
+    char *tab_buff= static_cast<char*>
+      (thd->alloc(ALIGN_SIZE(sizeof(TABLE_LIST)) * stab->lock_count));
+    char *key_buff= static_cast<char*>(thd->memdup(stab->qname.str,
+                                                   stab->qname.length));
+    if (!tab_buff || !key_buff)
       return;
 
     for (uint j= 0; j < stab->lock_count; j++)
     {
-      TABLE_LIST *table= (TABLE_LIST *)tab_buff;
-
-      table->db= key_buff;
-      table->db_length= stab->db_length;
-      table->table_name= table->db + table->db_length + 1;
-      table->table_name_length= stab->table_name_length;
-      table->alias= table->table_name + table->table_name_length + 1;
-      table->lock_type= stab->lock_type;
-      table->cacheable_table= 1;
-      table->prelocking_placeholder= 1;
-      table->belong_to_view= belong_to_view;
-      table->trg_event_map= stab->trg_event_map;
       /*
         Since we don't allow DDL on base tables in prelocked mode it
         is safe to infer the type of metadata lock from the type of
@@ -2140,7 +2129,7 @@ void sp_head::add_used_tables_to_table_list(THD *thd,
           acquire "strong" locks to ensure that LOCK TABLES properly
           works for storage engines which don't use THR_LOCK locks.
         */
-        mdl_lock_type= (table->lock_type >= TL_WRITE_ALLOW_WRITE) ?
+        mdl_lock_type= (stab->lock_type >= TL_WRITE_ALLOW_WRITE) ?
                        MDL_SHARED_NO_READ_WRITE : MDL_SHARED_READ_ONLY;
       }
       else
@@ -2150,12 +2139,21 @@ void sp_head::add_used_tables_to_table_list(THD *thd,
           Let us respect explicit LOW_PRIORITY clause if was used
           in the routine.
         */
-        mdl_lock_type= mdl_type_for_dml(table->lock_type);
+        mdl_lock_type= mdl_type_for_dml(stab->lock_type);
       }
 
-      MDL_REQUEST_INIT(&table->mdl_request,
-                       MDL_key::TABLE, table->db, table->table_name,
-                       mdl_lock_type, MDL_TRANSACTION);
+      TABLE_LIST *table= pointer_cast<TABLE_LIST*>(tab_buff);
+      table->init_one_table(key_buff, stab->db_length,
+                            key_buff + stab->db_length + 1,
+                            stab->table_name_length,
+                            key_buff + stab->db_length + 1 +
+                            stab->table_name_length + 1,
+                            stab->lock_type, mdl_lock_type);
+
+      table->cacheable_table= 1;
+      table->prelocking_placeholder= 1;
+      table->belong_to_view= belong_to_view;
+      table->trg_event_map= stab->trg_event_map;
 
       /* Everyting else should be zeroed */
 

sql/sql_trigger.cc

@@ -361,9 +361,7 @@ bool add_table_for_trigger(THD *thd,
   if (Trigger_loader::load_trn_file(thd, trigger_name, trn_path, &tbl_name))
     DBUG_RETURN(TRUE);
 
-  *table= sp_add_to_query_tables(thd, lex, db_name.str,
-                                 tbl_name.str, TL_IGNORE,
-                                 MDL_SHARED_NO_WRITE);
+  *table= sp_add_to_query_tables(thd, lex, db_name.str, tbl_name.str);
 
   DBUG_RETURN(*table ? FALSE : TRUE);
 }

sql/table.h

@@ -1707,7 +1707,8 @@ struct TABLE_LIST
                              const char *table_name_arg,
                              size_t table_name_length_arg,
                              const char *alias_arg,
-                             enum thr_lock_type lock_type_arg)
+                             enum thr_lock_type lock_type_arg,
+                             enum enum_mdl_type mdl_type_arg)
   {
     memset(this, 0, sizeof(*this));
     m_map= 1;
@@ -1719,13 +1720,26 @@ struct TABLE_LIST
     lock_type= lock_type_arg;
     MDL_REQUEST_INIT(&mdl_request,
                      MDL_key::TABLE, db, table_name,
-                     mdl_type_for_dml(lock_type),
+                     mdl_type_arg,
                      MDL_TRANSACTION);
     callback_func= 0;
     opt_hints_table= NULL;
     opt_hints_qb= NULL;
   }
 
+  inline void init_one_table(const char *db_name_arg,
+                             size_t db_length_arg,
+                             const char *table_name_arg,
+                             size_t table_name_length_arg,
+                             const char *alias_arg,
+                             enum thr_lock_type lock_type_arg)
+  {
+    init_one_table(db_name_arg, db_length_arg,
+                   table_name_arg, table_name_length_arg,
+                   alias_arg, lock_type_arg,
+                   mdl_type_for_dml(lock_type_arg));
+  }
+
   /// Create a TABLE_LIST object representing a nested join
   static TABLE_LIST *new_nested_join(MEM_ROOT *allocator,
                                      const char *alias,