Fixed bug#21390605: VALGRIND ERROR ON DELETE FROM TABLE CONTAINING AN INDEXED

VIRTUAL COLUMN

Take one test case for an example.
CREATE TABLE t1 (
 a INTEGER,
 b INTEGER GENERATED ALWAYS AS (a) VIRTUAL,
 c INTEGER GENERATED ALWAYS AS (b) VIRTUAL,
 INDEX idx (c));
When calling the callback function my_eval_gcolumn_expr_helper() to evaluate
the value of column 'c', the value of virtual generated column 'b' is needed.
But the caller only asks for 'c'. So the callback function won't calculate
column 'b' before evaluating column 'c'.  This results in invalid read of 'b'
during calculate 'c'.

The solution is to calculate all the base virtual generated columns before
evaluating the required column.

Instead of adding yet another walk() to gather needed columns, we do the
walk() a single time (when opening the table) and build a bitmap
(new member Generated_column::base_columns_map).  This bitmap replaces the
base_columns list. It is different as it lists virtual base columns too
(base_columns only listed *stored* base columns). So the InnoDB code, which
wants to see the stored base columns only, has been adapted to filter out the
bitmap; if it only needs a count of columns, it uses the new member
Generated_column::num_non_virtual_base_cols.

Author: Benny Wang

mysql-test/suite/gcol/inc/gcol_column_def_options.inc

@@ -307,6 +307,18 @@ INSERT INTO t1(col1,col2,col3,col4,col9,extra)
 VALUES(0,6,3,4,REPEAT(4,1000),0);
 ALTER TABLE t1 DROP COLUMN col1;
 DROP TABLE t1;
+
+--echo # Bug#21390605:VALGRIND ERROR ON DELETE FROM TABLE CONTAINING
+--echo #  AN INDEXED VIRTUAL COLUMN
+CREATE TABLE t1 (
+  a INTEGER,
+  b INTEGER GENERATED ALWAYS AS (a) VIRTUAL,
+  c INTEGER GENERATED ALWAYS AS (b) VIRTUAL,
+  INDEX idx (b,c)
+);
+INSERT INTO t1 (a) VALUES (42);
+DELETE FROM t1 WHERE c = 42;
+DROP TABLE t1;
 }
 
 --echo # Bug#20757211: GENERATED COLUMNS: ALTER TABLE CRASHES

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

@@ -406,6 +406,17 @@ INSERT INTO t1(col1,col2,col3,col4,col9,extra)
 VALUES(0,6,3,4,REPEAT(4,1000),0);
 ALTER TABLE t1 DROP COLUMN col1;
 DROP TABLE t1;
+# Bug#21390605:VALGRIND ERROR ON DELETE FROM TABLE CONTAINING
+#  AN INDEXED VIRTUAL COLUMN
+CREATE TABLE t1 (
+a INTEGER,
+b INTEGER GENERATED ALWAYS AS (a) VIRTUAL,
+c INTEGER GENERATED ALWAYS AS (b) VIRTUAL,
+INDEX idx (b,c)
+);
+INSERT INTO t1 (a) VALUES (42);
+DELETE FROM t1 WHERE c = 42;
+DROP TABLE t1;
 # Bug#20757211: GENERATED COLUMNS: ALTER TABLE CRASHES
 #  IN FIND_FIELD_IN_TABLE
 # 

sql/field.h

@@ -482,15 +482,13 @@ class Generated_column: public Sql_alloc
   LEX_STRING expr_str;
   /* It's used to free the items created in parsing generated expression */
   Item *item_free_list;
-  /*
-    A list of all stored (non-generated and stored generated) columns which a
-    generated column depends on. It is used by storage engines.
-  */
-  List<Field> base_columns;
+  /// Bitmap records base columns which a generated column depends on.
+  MY_BITMAP base_columns_map;
 
   Generated_column()
-    : expr_item(0), item_free_list(0), base_columns(),
-    field_type(MYSQL_TYPE_LONG), stored_in_db(false)
+    : expr_item(0), item_free_list(0),
+    field_type(MYSQL_TYPE_LONG),
+    stored_in_db(false), num_non_virtual_base_cols(0)
   {
     expr_str.str= NULL;
     expr_str.length= 0;
@@ -515,7 +513,13 @@ class Generated_column: public Sql_alloc
     stored_in_db= stored;
   }
   bool register_base_columns(TABLE *table);
+  /**
+    Get the number of non virtual base columns that this generated
+    column needs.
 
+    @return number of non virtual base columns
+  */
+  uint non_virtual_base_columns() const { return num_non_virtual_base_cols; }
 private:
   /*
     The following data is only updated by the parser and read
@@ -524,6 +528,8 @@ class Generated_column: public Sql_alloc
   enum_field_types field_type;   /* Real field type*/
   bool stored_in_db;             /* Indication that the field is 
                                     phisically stored in the database*/
+  /// How many non-virtual base columns in base_columns_map
+  uint num_non_virtual_base_cols;
 };
 
 class Proto_field

sql/handler.cc

@@ -7911,15 +7911,39 @@ static bool my_eval_gcolumn_expr_helper(THD *thd, TABLE *table,
                                                    blob_len_ptr_array);
 
   bool res= false;
+  MY_BITMAP fields_to_evaluate;
+  my_bitmap_map bitbuf[bitmap_buffer_size(MAX_FIELDS) / sizeof(my_bitmap_map)];
+  bitmap_init(&fields_to_evaluate, bitbuf, table->s->fields, 0);
+  bitmap_set_all(&fields_to_evaluate);
+  bitmap_intersect(&fields_to_evaluate, fields);
+  /*
+    In addition to evaluating the value for the columns requested by
+    the caller we also need to evaluate any virtual columns that these
+    depend on.
+    This loop goes through the columns that should be evaluated and
+    adds all the base columns. If the base column is virtual, it has
+    to be evaluated.
+  */
   for (Field **vfield_ptr= table->vfield; *vfield_ptr; vfield_ptr++)
   {
     Field *field= *vfield_ptr;
-
     // Validate that the field number is less than the bit map size
     DBUG_ASSERT(field->field_index < fields->n_bits);
 
-    // Check if we should evaluate this field
     if (bitmap_is_set(fields, field->field_index))
+      bitmap_union(&fields_to_evaluate, &field->gcol_info->base_columns_map);
+  }
+
+   /*
+     Evaluate all requested columns and all base columns these depends
+     on that are virtual.
+  */
+  for (Field **vfield_ptr= table->vfield; *vfield_ptr; vfield_ptr++)
+  {
+    Field *field= *vfield_ptr;
+
+    // Check if we should evaluate this field
+    if (bitmap_is_set(&fields_to_evaluate, field->field_index))
     {
       DBUG_ASSERT(field->gcol_info && field->gcol_info->expr_item->fixed);
       if (in_purge)

sql/table.cc

@@ -2721,7 +2721,8 @@ bool fix_fields_gcol_func(THD *thd, Field *field)
 }
 
 /**
-  Build the base_columns member of this generated column
+  Calculate the base_columns_map and num_non_virtual_base_cols members of
+  this generated column
 
   @param table    Table with the checked field
 
@@ -2731,8 +2732,10 @@ bool fix_fields_gcol_func(THD *thd, Field *field)
 bool Generated_column::register_base_columns(TABLE *table)
 {
   DBUG_ENTER("register_base_columns");
-  my_bitmap_map bitbuf[bitmap_buffer_size(MAX_FIELDS) / sizeof(my_bitmap_map)];
-  MY_BITMAP base_columns_map;
+  my_bitmap_map *bitbuf=
+    static_cast<my_bitmap_map *>(alloc_root(&table->mem_root,
+                                bitmap_buffer_size(table->s->fields)));
+  DBUG_ASSERT(num_non_virtual_base_cols == 0);
   bitmap_init(&base_columns_map, bitbuf, table->s->fields, 0);
 
   MY_BITMAP *save_old_read_set= table->read_set;
@@ -2741,14 +2744,14 @@ bool Generated_column::register_base_columns(TABLE *table)
   expr_item->walk(&Item::mark_field_in_map,
                   Item::WALK_PREFIX, (uchar *) &mark_fld);
   table->read_set= save_old_read_set;
+
+  /* Calculate the number of non-virtual base columns */
   for (uint i= 0; i < table->s->fields; i++)
   {
     Field *field= table->field[i];
-    /* Virtual generated columns are not needed */
-    if (field->stored_in_db &&
-        bitmap_is_set(&base_columns_map, field->field_index) &&
-        base_columns.push_back(field))
-      DBUG_RETURN(true); // OOM
+    if (bitmap_is_set(&base_columns_map, field->field_index) &&
+        field->stored_in_db)
+      num_non_virtual_base_cols++;
   }
   DBUG_RETURN(false);
 }
@@ -6613,12 +6616,6 @@ void TABLE::mark_generated_columns(bool is_update)
     {
       tmp_vfield= *vfield_ptr;
       DBUG_ASSERT(tmp_vfield->gcol_info && tmp_vfield->gcol_info->expr_item);
-      Mark_field mark_fld(MARK_COLUMNS_TEMP);
-      MY_BITMAP *save_old_read_set= read_set;
-      bitmap_clear_all(&dependent_fields);
-      read_set= &dependent_fields;
-      tmp_vfield->gcol_info->expr_item->walk(&Item::mark_field_in_map,
-                              Item::WALK_PREFIX, (uchar *) &mark_fld);
 
       /*
         We need to evaluate the GC if:
@@ -6632,9 +6629,9 @@ void TABLE::mark_generated_columns(bool is_update)
            be located first, for that the GC's value is needed.
       */
       if ((!tmp_vfield->stored_in_db && tmp_vfield->m_indexed) ||
-          bitmap_is_overlapping(read_set, write_set))
+          bitmap_is_overlapping(write_set,
+                                &tmp_vfield->gcol_info->base_columns_map))
       {
-        read_set= save_old_read_set;
         // The GC needs to be updated
         tmp_vfield->table->mark_column_used(in_use, tmp_vfield,
                                             MARK_COLUMNS_WRITE);
@@ -6643,8 +6640,6 @@ void TABLE::mark_generated_columns(bool is_update)
                                             MARK_COLUMNS_READ);
         bitmap_updated= TRUE;
       }
-      else
-        read_set= save_old_read_set;
     }
   }
   else // Insert needs to evaluate all generated columns
@@ -7570,9 +7565,9 @@ void TABLE::mark_gcol_in_maps(Field *field)
 {
   bitmap_set_bit(write_set, field->field_index);
   /*
-    Using MARK_COLUMNS_TEMP has the effect that underlying base columns
-    are added to read_set but not added to requirements for an index to be
-    covering (covering_keys is not touched). So, if we have:
+    Note that underlying base columns are here added to read_set but not added
+    to requirements for an index to be covering (covering_keys is not touched).
+    So, if we have:
     SELECT gcol FROM t :
     - an index covering gcol only (not including base columns), can still be
     chosen by the optimizer; note that InnoDB's build_template_needs_field()
@@ -7582,7 +7577,13 @@ void TABLE::mark_gcol_in_maps(Field *field)
     they are in read_set.
     - Note how this relies on InnoDB's behaviour.
   */
-  Mark_field mf(this, MARK_COLUMNS_TEMP);
-  field->gcol_info->expr_item->walk(&Item::mark_field_in_map,Item::WALK_PREFIX,
-                                    reinterpret_cast<uchar *>(&mf));
+  for (uint i= 0; i < s->fields; i++)
+  {
+    if (bitmap_is_set(&field->gcol_info->base_columns_map, i))
+    {
+      bitmap_set_bit(read_set, i);
+      if (this->field[i]->is_virtual_gcol())
+        bitmap_set_bit(write_set, i);
+    }
+  }
 }

storage/innobase/handler/ha_innodb.cc

@@ -9259,26 +9259,28 @@ innodb_base_col_setup(
 	dict_v_col_t*	v_col)
 {
 	int     n = 0;
-	const Field* base_field;
 
-	List_iterator<Field>    iter(field->gcol_info->base_columns);
+	for (uint i= 0; i < field->table->s->fields; ++i) {
+		const Field* base_field = field->table->field[i];
 
-	while ((base_field = iter++)) {
-		ulint   z;
+		if (!base_field->is_virtual_gcol()
+        && bitmap_is_set(&field->gcol_info->base_columns_map, i)) {
+			ulint   z;
 
-		for (z = 0; z < table->n_cols; z++) {
-			const char*     name = dict_table_get_col_name(table, z);
-			if (!innobase_strcasecmp(name,
-						 base_field->field_name)) {
-				break;
+			for (z = 0; z < table->n_cols; z++) {
+				const char* name = dict_table_get_col_name(table, z);
+				if (!innobase_strcasecmp(name,
+																 base_field->field_name)) {
+					break;
+				}
 			}
-		}
 
-		ut_ad(z != table->n_cols);
+			ut_ad(z != table->n_cols);
 
-		v_col->base_col[n] = dict_table_get_nth_col(table, z);
-		ut_ad(v_col->base_col[n]->ind == z);
-		n++;
+			v_col->base_col[n] = dict_table_get_nth_col(table, z);
+			ut_ad(v_col->base_col[n]->ind == z);
+			n++;
+		}
 	}
 }
 
@@ -9525,7 +9527,7 @@ create_table_info_t::create_table_def()
 					| is_virtual,
 					charset_no),
 				col_len, i,
-				field->gcol_info->base_columns.elements);
+				field->gcol_info->non_virtual_base_columns());
 		}
 	}
 

storage/innobase/handler/handler0alter.cc

@@ -3190,7 +3190,7 @@ prepare_inplace_add_virtual(
 
 		ctx->add_vcol[j].m_col.ind = i - 1;
 		ctx->add_vcol[j].num_base =
-		  field->gcol_info->base_columns.elements;
+		  field->gcol_info->non_virtual_base_columns();
 		ctx->add_vcol_name[j] = field->field_name;
 		ctx->add_vcol[j].base_col = static_cast<dict_col_t**>(
 			mem_heap_alloc(ctx->heap, ctx->add_vcol[j].num_base
@@ -4086,7 +4086,7 @@ prepare_inplace_alter_table_dict(
 						field_type, charset_no)
 					| DATA_VIRTUAL,
 					col_len, i,
-					field->gcol_info->base_columns.elements);
+					field->gcol_info->non_virtual_base_columns());
 			} else {
 				dict_mem_table_add_col(
 					ctx->new_table, ctx->heap,