Bug#22133710 GCOLS: READ UNCOMMITTED: ASSERT !TABLE || (!TABLE->WRITE_SET || BITMAP_IS_SET(TA

Problem: in READ UNCOMMITTED isolation, a SELECT reading an indexed
generated column could trigger an assertion failure in debug binaries.

my_eval_gcolumn_expr*() is called by the storage engine, which may request to
evaluate more generated columns than read_set/write_set says.
For example, InnoDB's row_sel_sec_rec_is_for_clust_rec() reads the full
record from the clustered index and asks us to compute generated columns
that match key fields in the used secondary index. So we trust that the
engine has filled all base columns necessary to requested computations,
and we ignore read_set/write_set.
The in_purge branch becomes unneeded.
I noticed that the my_rec argument of innobase_get_computed_value()
is always NULL, so in agreement with Jimmy I remove it.

Author: Guilhem Bichot

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

@@ -609,3 +609,20 @@ SELECT @c2 - @c1;
 @c2 - @c1
 0
 DROP TABLE t;
+#
+# Bug#22133710 GCOLS: READ UNCOMMITTED: ASSERT !TABLE || (!TABLE->WRITE_SET || BITMAP_IS_SET(TA
+#
+CREATE TABLE t (
+a INT,
+b INT GENERATED ALWAYS AS (1) VIRTUAL,
+c INT GENERATED ALWAYS AS (1) VIRTUAL,
+d INT GENERATED ALWAYS AS (1) VIRTUAL,
+KEY (b,d)
+) ENGINE=INNODB;
+INSERT INTO t VALUES();
+SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED ;
+SELECT 1 FROM t WHERE c GROUP BY b;
+1
+1
+COMMIT;
+DROP TABLE t;

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

@@ -581,3 +581,20 @@ DO (SELECT (@c2:= c) - a FROM t);
 SELECT @c2 - @c1;
 
 DROP TABLE t;
+
+--echo #
+--echo # Bug#22133710 GCOLS: READ UNCOMMITTED: ASSERT !TABLE || (!TABLE->WRITE_SET || BITMAP_IS_SET(TA
+--echo #
+
+CREATE TABLE t (
+  a INT,
+  b INT GENERATED ALWAYS AS (1) VIRTUAL,
+  c INT GENERATED ALWAYS AS (1) VIRTUAL,
+  d INT GENERATED ALWAYS AS (1) VIRTUAL,
+  KEY (b,d)
+) ENGINE=INNODB;
+INSERT INTO t VALUES();
+SET TRANSACTION ISOLATION LEVEL READ UNCOMMITTED ;
+SELECT 1 FROM t WHERE c GROUP BY b;
+COMMIT;
+DROP TABLE t;

sql/handler.cc

@@ -8087,7 +8087,7 @@ static bool my_eval_gcolumn_expr_helper(THD *thd, TABLE *table,
   repoint_field_to_record(table, old_buf, record);
 
   blob_len_ptr blob_len_ptr_array[MAX_FIELDS];
-  
+
   /*
     If it's purge thread, we need get the space allocated by storage engine
     for blob.
@@ -8123,7 +8123,20 @@ static bool my_eval_gcolumn_expr_helper(THD *thd, TABLE *table,
    /*
      Evaluate all requested columns and all base columns these depends
      on that are virtual.
+
+     This function is called by the storage engine, which may request to
+     evaluate more generated columns than read_set/write_set says.
+     For example, InnoDB's row_sel_sec_rec_is_for_clust_rec() reads the full
+     record from the clustered index and asks us to compute generated columns
+     that match key fields in the used secondary index. So we trust that the
+     engine has filled all base columns necessary to requested computations,
+     and we ignore read_set/write_set.
   */
+
+  my_bitmap_map *old_maps[2];
+  dbug_tmp_use_all_columns(table, old_maps,
+                           table->read_set, table->write_set);
+
   for (Field **vfield_ptr= table->vfield; *vfield_ptr; vfield_ptr++)
   {
     Field *field= *vfield_ptr;
@@ -8133,16 +8146,6 @@ static bool my_eval_gcolumn_expr_helper(THD *thd, TABLE *table,
         field->is_virtual_gcol())
     {
       DBUG_ASSERT(field->gcol_info && field->gcol_info->expr_item->fixed);
-      if (in_purge)
-      {
-        /*
-          Adding to read_set/write_set is normally done up-front by high-level
-          layers before calling any handler's read/delete/etc functions.
-          But, for the specific case of the purge thread, it has no high-level
-          layer to manage read_set/write_set.
-        */
-        table->mark_column_used(thd, field, MARK_COLUMNS_WRITE);
-      }
 
       const type_conversion_status save_in_field_status=
         field->gcol_info->expr_item->save_in_field(field, 0);
@@ -8163,6 +8166,8 @@ static bool my_eval_gcolumn_expr_helper(THD *thd, TABLE *table,
     }
   }
 
+  dbug_tmp_restore_column_maps(table->read_set, table->write_set, old_maps);
+
   /*
     If it's a purge thread, we need copy the blob data into specified place
     allocated by storage engine so that the blob data still can be accessed

storage/innobase/handler/ha_innodb.cc

@@ -19953,7 +19953,6 @@ innobase_init_vc_templ(
 @param[in,out]	row		the data row
 @param[in]	col		virtual column
 @param[in]	index		index
-@param[in,out]	my_rec		mysql record to store the data
 @param[in,out]	local_heap	heap memory for processing large data etc.
 @param[in,out]	heap		memory heap that copies the actual index row
 @param[in]	ifield		index field
@@ -19965,7 +19964,6 @@ innobase_get_computed_value(
 	const dtuple_t*		row,
 	const dict_v_col_t*	col,
 	const dict_index_t*	index,
-	byte*			my_rec,
 	mem_heap_t**		local_heap,
 	mem_heap_t*		heap,
 	const dict_field_t*	ifield,
@@ -19992,22 +19990,12 @@ innobase_get_computed_value(
 			*local_heap = mem_heap_create(UNIV_PAGE_SIZE);
 		}
 
-		if (!my_rec) {
-			mysql_rec = static_cast<byte*>(mem_heap_alloc(
-				*local_heap, index->table->vc_templ->rec_len));
-		} else {
-			mysql_rec = my_rec;
-		}
-
+		mysql_rec = static_cast<byte*>(mem_heap_alloc(
+			    *local_heap, index->table->vc_templ->rec_len));
 		buf = static_cast<byte*>(mem_heap_alloc(
 				*local_heap, index->table->vc_templ->rec_len));
 	} else {
-		if (!my_rec) {
-			mysql_rec = rec_buf1;
-		} else {
-			mysql_rec = my_rec;
-		}
-
+		mysql_rec = rec_buf1;
 		buf = rec_buf2;
 	}
 
@@ -20112,7 +20100,7 @@ innobase_get_computed_value(
 	}
 
 	/* we just want to store the data in passed in MySQL record */
-	if (my_rec || ret != 0) {
+	if (ret != 0) {
 		return(NULL);
 	}
 

storage/innobase/include/row0mysql.h

@@ -914,7 +914,6 @@ struct SysIndexCallback {
 @param[in,out]	row		the data row
 @param[in]	col		virtual column
 @param[in]	index		index on the virtual column
-@param[in,out]	my_rec		MySQL record to store the rows
 @param[in,out]	local_heap	heap memory for processing large data etc.
 @param[in,out]	heap		memory heap that copies the actual index row
 @param[in]	ifield		index field
@@ -925,7 +924,6 @@ innobase_get_computed_value(
 	const dtuple_t*		row,
 	const dict_v_col_t*	col,
 	const dict_index_t*	index,
-	byte*			my_rec,
 	mem_heap_t**		local_heap,
 	mem_heap_t*		heap,
 	const dict_field_t*	ifield,

storage/innobase/row/row0merge.cc

@@ -581,7 +581,7 @@ row_merge_buf_add(
 					= dict_table_get_first_index(new_table);
 
 				row_field = innobase_get_computed_value(
-					row, v_col, clust_index, NULL,
+					row, v_col, clust_index,
 					v_heap, NULL, ifield, false);
 
 				if (row_field == NULL) {

storage/innobase/row/row0sel.cc

@@ -231,7 +231,7 @@ row_sel_sec_rec_is_for_clust_rec(
 
 			vfield = innobase_get_computed_value(
 					row, v_col, clust_index,
-					NULL, &heap, NULL, NULL, false);
+					&heap, NULL, NULL, false);
 
 			clust_len = vfield->len;
 			clust_field = static_cast<byte*>(vfield->data);

storage/innobase/row/row0upd.cc

@@ -928,7 +928,7 @@ row_upd_build_difference_binary(
 			dfield = dtuple_get_nth_v_field(entry, i);
 
 			dfield_t*	vfield = innobase_get_computed_value(
-				update->old_vrow, col, index, NULL,
+				update->old_vrow, col, index,
 				&v_heap, heap, NULL, false);
 
 			if (!dfield_data_is_binary_equal(
@@ -1929,7 +1929,7 @@ row_upd_store_v_row(
 					/* Need to compute, this happens when
 					deleting row */
 					innobase_get_computed_value(
-						node->row, col, index, NULL,
+						node->row, col, index,
 						&heap, node->heap, NULL,
 						false);
 				}

storage/innobase/row/row0vers.cc

@@ -492,7 +492,7 @@ row_vers_build_clust_v_col(
 				ind_field->col);
 
 			innobase_get_computed_value(
-				row, col, clust_index, NULL, &local_heap,
+				row, col, clust_index, &local_heap,
 				heap, NULL, true);
 		}
 	}