MDEV-18366 Crash on SELECT on a table with indexed virtual columns

The problem was in improper error handling behavior in
`row_upd_build_difference_binary`:
`innobase_free_row_for_vcol` wasn't called.

To eliminate this problem in all potential places, a refactoring has been
made:
* class ib_vcol_row is added. It owns VCOL_STORAGE and heap and maintains
 it in RAII manner
* all innobase_allocate_row_for_vcol/innobase_free_row_for_vcol pairs are
 substituted
 with ib_vcol_row usage
* row_merge_buf_add is only left untouched because it doesn't own vheap
 passed as an argument
* innobase_allocate_row_for_vcol does not allocate VCOL_STORAGE anymore and
 accepts it as an argument -- this reduces a number of memory allocations
 * move rec_printer out of `#ifndef DBUG_OFF` and mark it cold

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

@@ -259,6 +259,9 @@ ERROR 22007: Incorrect date value: '20190132' for column `test`.`t1`.`vb` at row
 SELECT * FROM t1;
 a	b	vb
 ROLLBACK;
+SELECT * FROM t1;
+a	b	vb
+1	20190132	0000-00-00
 CHECK TABLE t1;
 Table	Op	Msg_type	Msg_text
 test.t1	check	status	OK

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

@@ -278,7 +278,6 @@ DELETE FROM t1;
 INSERT INTO t1 (a,b) VALUES(1,20190123);
 SELECT * FROM t1;
 ROLLBACK;
-# MDEV-18366 FIXME: fix the crash and enable this
-# SELECT * FROM t1;
+SELECT * FROM t1;
 CHECK TABLE t1;
 DROP TABLE t1;

storage/innobase/handler/ha_innodb.cc

@@ -21820,64 +21820,53 @@ innobase_get_field_from_update_vector(
    Allocate a heap and record for calculating virtual fields
    Used mainly for virtual fields in indexes
 
-@param[in]	thd		MariaDB THD
-@param[in]	index		Index in use
-@param[out]	heap		Heap that holds temporary row
-@param[in,out]	table		MariaDB table
-@param[out]	record		Pointer to allocated MariaDB record
-@param[out]	storage		Internal storage for blobs etc
-
-@retval		false on success
-@retval		true on malloc failure or failed to open the maria table
+@param[in]      thd             MariaDB THD
+@param[in]      index           Index in use
+@param[out]     heap            Heap that holds temporary row
+@param[in,out]  table           MariaDB table
+@param[out]     record	        Pointer to allocated MariaDB record
+@param[out]     storage	        Internal storage for blobs etc
+
+@retval		true on success
+@retval		false on malloc failure or failed to open the maria table
 		for purge thread.
 */
 
-bool innobase_allocate_row_for_vcol(
-				    THD *	  thd,
-				    dict_index_t* index,
-				    mem_heap_t**  heap,
-				    TABLE**	  table,
-				    byte**	  record,
-				    VCOL_STORAGE** storage)
-{
-	TABLE *maria_table;
-	String *blob_value_storage;
-	if (!*table)
-		*table= innodb_find_table_for_vc(thd, index->table);
-
-	/* For purge thread, there is a possiblity that table could have
-	dropped, corrupted or unaccessible. */
-	if (!*table)
-		return true;
-	maria_table= *table;
-	if (!*heap && !(*heap= mem_heap_create(srv_page_size)))
-	{
-		*storage= 0;
-		return TRUE;
-	}
-	*record= static_cast<byte*>(mem_heap_alloc(*heap,
-                                                   maria_table->s->reclength));
-	*storage= static_cast<VCOL_STORAGE*>
-          (mem_heap_alloc(*heap, sizeof(**storage)));
-	blob_value_storage= static_cast<String*>
-          (mem_heap_alloc(*heap,
-                          maria_table->s->virtual_not_stored_blob_fields *
-                          sizeof(String)));
-	if (!*record || !*storage || !blob_value_storage)
-	{
-		*storage= 0;
-		return TRUE;
-	}
-	(*storage)->maria_table= maria_table;
-	(*storage)->innobase_record= *record;
-	(*storage)->maria_record= maria_table->field[0]->record_ptr();
-	(*storage)->blob_value_storage= blob_value_storage;
+bool innobase_allocate_row_for_vcol(THD *thd, dict_index_t *index,
+                                    mem_heap_t **heap, TABLE **table,
+                                    VCOL_STORAGE *storage)
+{
+  TABLE *maria_table;
+  String *blob_value_storage;
+  if (!*table)
+    *table = innodb_find_table_for_vc(thd, index->table);
+
+  /* For purge thread, there is a possiblity that table could have
+     dropped, corrupted or unaccessible. */
+  if (!*table)
+    return false;
+  maria_table = *table;
+  if (!*heap && !(*heap = mem_heap_create(srv_page_size)))
+    return false;
 
-	maria_table->move_fields(maria_table->field, *record,
-				 (*storage)->maria_record);
-	maria_table->remember_blob_values(blob_value_storage);
+  uchar *record = static_cast<byte *>(mem_heap_alloc(*heap,
+                                                    maria_table->s->reclength));
 
-	return FALSE;
+  size_t len = maria_table->s->virtual_not_stored_blob_fields * sizeof(String);
+  blob_value_storage = static_cast<String *>(mem_heap_alloc(*heap, len));
+
+  if (!record || !blob_value_storage)
+    return false;
+
+  storage->maria_table = maria_table;
+  storage->innobase_record = record;
+  storage->maria_record = maria_table->field[0]->record_ptr();
+  storage->blob_value_storage = blob_value_storage;
+
+  maria_table->move_fields(maria_table->field, record, storage->maria_record);
+  maria_table->remember_blob_values(blob_value_storage);
+
+  return true;
 }
 
 

storage/innobase/include/rem0rec.h

@@ -1076,16 +1076,17 @@ struct rec_offsets_print
 @param[in,out]	o	output stream
 @param[in]	r	record to display
 @return	the output stream */
+ATTRIBUTE_COLD
 std::ostream&
 operator<<(std::ostream& o, const rec_offsets_print& r);
 
-# ifndef DBUG_OFF
 /** Pretty-printer of records and tuples */
 class rec_printer : public std::ostringstream {
 public:
 	/** Construct a pretty-printed record.
 	@param rec	record with header
 	@param offsets	rec_get_offsets(rec, ...) */
+	ATTRIBUTE_COLD
 	rec_printer(const rec_t* rec, const rec_offs* offsets)
 		:
 		std::ostringstream ()
@@ -1099,6 +1100,7 @@ class rec_printer : public std::ostringstream {
 	@param rec record, possibly lacking header
 	@param info rec_get_info_bits(rec)
 	@param offsets rec_get_offsets(rec, ...) */
+	ATTRIBUTE_COLD
 	rec_printer(const rec_t* rec, ulint info, const rec_offs* offsets)
 		:
 		std::ostringstream ()
@@ -1108,6 +1110,7 @@ class rec_printer : public std::ostringstream {
 
 	/** Construct a pretty-printed tuple.
 	@param tuple	data tuple */
+	ATTRIBUTE_COLD
 	rec_printer(const dtuple_t* tuple)
 		:
 		std::ostringstream ()
@@ -1118,6 +1121,7 @@ class rec_printer : public std::ostringstream {
 	/** Construct a pretty-printed tuple.
 	@param field	array of data tuple fields
 	@param n	number of fields */
+	ATTRIBUTE_COLD
 	rec_printer(const dfield_t* field, ulint n)
 		:
 		std::ostringstream ()
@@ -1134,7 +1138,7 @@ class rec_printer : public std::ostringstream {
 	/** Assignment operator */
 	rec_printer& operator=(const rec_printer& other);
 };
-# endif /* !DBUG_OFF */
+
 
 # ifdef UNIV_DEBUG
 /** Read the DB_TRX_ID of a clustered index record.

storage/innobase/include/row0mysql.h

@@ -814,6 +814,8 @@ struct VCOL_STORAGE
 	byte *innobase_record;
 	byte *maria_record;
 	String *blob_value_storage;
+	VCOL_STORAGE(): maria_table(NULL), innobase_record(NULL),
+		maria_record(NULL),  blob_value_storage(NULL) {}
 };
 
 /**
@@ -836,12 +838,42 @@ bool innobase_allocate_row_for_vcol(
 				    dict_index_t* index,
 				    mem_heap_t**  heap,
 				    TABLE**	  table,
-				    byte**	  record,
-				    VCOL_STORAGE** storage);
+				    VCOL_STORAGE* storage);
 
 /** Free memory allocated by innobase_allocate_row_for_vcol() */
 void innobase_free_row_for_vcol(VCOL_STORAGE *storage);
 
+class ib_vcol_row
+{
+  VCOL_STORAGE storage;
+public:
+  mem_heap_t *heap;
+
+  ib_vcol_row(mem_heap_t *heap) : heap(heap) {}
+
+  byte *record(THD *thd, dict_index_t *index, TABLE **table)
+  {
+    if (!storage.innobase_record)
+    {
+      bool ok = innobase_allocate_row_for_vcol(thd, index, &heap, table,
+                                               &storage);
+      if (!ok)
+        return NULL;
+    }
+    return storage.innobase_record;
+  };
+
+  ~ib_vcol_row()
+  {
+    if (heap)
+    {
+      if (storage.innobase_record)
+        innobase_free_row_for_vcol(&storage);
+      mem_heap_free(heap);
+    }
+  }
+};
+
 /** Get the computed value by supplying the base column values.
 @param[in,out]	row		the data row
 @param[in]	col		virtual column

storage/innobase/row/row0ins.cc

@@ -916,16 +916,15 @@ row_ins_invalidate_query_cache(
 @param[in]	index		clustered index of child table
 @param[in]	node		parent update node
 @param[in]	foreign		foreign key information
-@param[out]	err		error code. */
+@return		error code. */
 static
-void
+dberr_t
 row_ins_foreign_fill_virtual(
 	upd_node_t*		cascade,
 	const rec_t*		rec,
 	dict_index_t*		index,
 	upd_node_t*		node,
-	dict_foreign_t*		foreign,
-	dberr_t*		err)
+	dict_foreign_t*		foreign)
 {
 	THD*		thd = current_thd;
 	row_ext_t*	ext;
@@ -934,10 +933,7 @@ row_ins_foreign_fill_virtual(
 	const rec_offs*	offsets =
 		rec_get_offsets(rec, index, offsets_, true,
 				ULINT_UNDEFINED, &cascade->heap);
-	mem_heap_t*	v_heap = NULL;
 	TABLE*		mysql_table= NULL;
-	VCOL_STORAGE*	vcol_storage= NULL;
-	byte*		record;
 	upd_t*		update = cascade->update;
 	ulint		n_v_fld = index->table->n_v_def;
 	ulint		n_diff;
@@ -957,12 +953,10 @@ row_ins_foreign_fill_virtual(
 		innobase_init_vc_templ(index->table);
 	}
 
-	if (innobase_allocate_row_for_vcol(thd, index, &v_heap,
-                                           &mysql_table,
-                                           &record, &vcol_storage)) {
-		if (v_heap) mem_heap_free(v_heap);
-		*err = DB_OUT_OF_MEMORY;
-		goto func_exit;
+	ib_vcol_row vc(NULL);
+	uchar *record = vc.record(thd, index, &mysql_table);
+	if (!record) {
+		return DB_OUT_OF_MEMORY;
 	}
 
 	for (ulint i = 0; i < n_v_fld; i++) {
@@ -978,12 +972,11 @@ row_ins_foreign_fill_virtual(
 
 		dfield_t*	vfield = innobase_get_computed_value(
 				update->old_vrow, col, index,
-				&v_heap, update->heap, NULL, thd, mysql_table,
+				&vc.heap, update->heap, NULL, thd, mysql_table,
                                 record, NULL, NULL, NULL);
 
 		if (vfield == NULL) {
-			*err = DB_COMPUTE_VALUE_FAILED;
-			goto func_exit;
+			return DB_COMPUTE_VALUE_FAILED;
 		}
 
 		upd_field = upd_get_nth_field(update, n_diff);
@@ -1008,13 +1001,12 @@ row_ins_foreign_fill_virtual(
 
 			dfield_t* new_vfield = innobase_get_computed_value(
 					update->old_vrow, col, index,
-					&v_heap, update->heap, NULL, thd,
+					&vc.heap, update->heap, NULL, thd,
 					mysql_table, record, NULL,
 					node->update, foreign);
 
 			if (new_vfield == NULL) {
-				*err = DB_COMPUTE_VALUE_FAILED;
-				goto func_exit;
+				return DB_COMPUTE_VALUE_FAILED;
 			}
 
 			dfield_copy(&(upd_field->new_val), new_vfield);
@@ -1024,14 +1016,7 @@ row_ins_foreign_fill_virtual(
 	}
 
 	update->n_fields = n_diff;
-	*err = DB_SUCCESS;
-
-func_exit:
-	if (v_heap) {
-		if (vcol_storage)
-			innobase_free_row_for_vcol(vcol_storage);
-		mem_heap_free(v_heap);
-	}
+	return DB_SUCCESS;
 }
 
 #ifdef WITH_WSREP
@@ -1312,9 +1297,9 @@ row_ins_foreign_check_on_constraint(
 
 		if (foreign->v_cols != NULL
 		    && foreign->v_cols->size() > 0) {
-			row_ins_foreign_fill_virtual(
+			err = row_ins_foreign_fill_virtual(
 				cascade, clust_rec, clust_index,
-				node, foreign, &err);
+				node, foreign);
 
 			if (err != DB_SUCCESS) {
 				goto nonstandard_exit_func;
@@ -1351,9 +1336,9 @@ row_ins_foreign_check_on_constraint(
 			node, foreign, tmp_heap, trx);
 
 		if (foreign->v_cols && !foreign->v_cols->empty()) {
-			row_ins_foreign_fill_virtual(
+			err = row_ins_foreign_fill_virtual(
 				cascade, clust_rec, clust_index,
-				node, foreign, &err);
+				node, foreign);
 
 			if (err != DB_SUCCESS) {
 				goto nonstandard_exit_func;