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;

storage/innobase/row/row0merge.cc

@@ -520,8 +520,7 @@ row_merge_buf_add(
 	ulint			bucket = 0;
 	doc_id_t		write_doc_id;
 	ulint			n_row_added = 0;
-	VCOL_STORAGE*		vcol_storage= 0;
-	byte*			record;
+	VCOL_STORAGE		vcol_storage;
 	DBUG_ENTER("row_merge_buf_add");
 
 	if (buf->n_tuples >= buf->max_tuples) {
@@ -594,8 +593,11 @@ row_merge_buf_add(
 				dict_index_t*	clust_index
 					= dict_table_get_first_index(new_table);
 
-                                if (!vcol_storage &&
-                                    innobase_allocate_row_for_vcol(trx->mysql_thd, clust_index, v_heap, &my_table, &record, &vcol_storage)) {
+                                if (!vcol_storage.innobase_record &&
+                                    !innobase_allocate_row_for_vcol(
+						trx->mysql_thd, clust_index,
+						v_heap, &my_table,
+						&vcol_storage)) {
 					*err = DB_OUT_OF_MEMORY;
 					goto error;
 				}
@@ -603,8 +605,8 @@ row_merge_buf_add(
 				row_field = innobase_get_computed_value(
 					row, v_col, clust_index,
 					v_heap, NULL, ifield, trx->mysql_thd,
-					my_table, record, old_table, NULL,
-					NULL);
+					my_table, vcol_storage.innobase_record, 
+					old_table, NULL, NULL);
 
 				if (row_field == NULL) {
 					*err = DB_COMPUTE_VALUE_FAILED;
@@ -846,13 +848,13 @@ row_merge_buf_add(
 	}
 
 end:
-        if (vcol_storage)
-		innobase_free_row_for_vcol(vcol_storage);
+        if (vcol_storage.innobase_record)
+		innobase_free_row_for_vcol(&vcol_storage);
 	DBUG_RETURN(n_row_added);
 
 error:
-        if (vcol_storage)
-		innobase_free_row_for_vcol(vcol_storage);
+        if (vcol_storage.innobase_record)
+		innobase_free_row_for_vcol(&vcol_storage);
         DBUG_RETURN(0);
 }
 

storage/innobase/row/row0sel.cc

@@ -176,9 +176,6 @@ row_sel_sec_rec_is_for_clust_rec(
 	rec_offs	sec_offsets_[REC_OFFS_SMALL_SIZE];
 	rec_offs*	clust_offs	= clust_offsets_;
 	rec_offs*	sec_offs	= sec_offsets_;
-	ibool		is_equal	= TRUE;
-	VCOL_STORAGE*	vcol_storage= 0;
-	byte*		record;
 
 	rec_offs_init(clust_offsets_);
 	rec_offs_init(sec_offsets_);
@@ -197,6 +194,7 @@ row_sel_sec_rec_is_for_clust_rec(
 	}
 
 	heap = mem_heap_create(256);
+	ib_vcol_row vc(heap);
 
 	clust_offs = rec_get_offsets(clust_rec, clust_index, clust_offs,
 				     true, ULINT_UNDEFINED, &heap);
@@ -225,16 +223,9 @@ row_sel_sec_rec_is_for_clust_rec(
 			dfield_t*		vfield;
 			row_ext_t*		ext;
 
-			if (!vcol_storage)
-			{
-				TABLE *mysql_table= thr->prebuilt->m_mysql_table;
-				innobase_allocate_row_for_vcol(thr_get_trx(thr)->mysql_thd,
-							       clust_index,
-							       &heap,
-							       &mysql_table,
-							       &record,
-							       &vcol_storage);
-			}
+			byte *record = vc.record(thr_get_trx(thr)->mysql_thd,
+						 clust_index,
+						 &thr->prebuilt->m_mysql_table);
 
 			v_col = reinterpret_cast<const dict_v_col_t*>(col);
 
@@ -284,7 +275,7 @@ row_sel_sec_rec_is_for_clust_rec(
 					    sec_field, sec_len,
 					    ifield->prefix_len,
 					    clust_index->table)) {
-					goto inequal;
+					return FALSE;
 				}
 
 				continue;
@@ -320,28 +311,19 @@ row_sel_sec_rec_is_for_clust_rec(
 			rtr_read_mbr(sec_field, &sec_mbr);
 
 			if (!MBR_EQUAL_CMP(&sec_mbr, &tmp_mbr)) {
-				is_equal = FALSE;
-				goto func_exit;
+				return FALSE;
 			}
 		} else {
 
 			if (0 != cmp_data_data(col->mtype, col->prtype,
 					       clust_field, len,
 					       sec_field, sec_len)) {
-inequal:
-				is_equal = FALSE;
-				goto func_exit;
+				return FALSE;
 			}
 		}
 	}
 
-func_exit:
-	if (UNIV_LIKELY_NULL(heap)) {
-		if (UNIV_LIKELY_NULL(vcol_storage))
-			innobase_free_row_for_vcol(vcol_storage);
-		mem_heap_free(heap);
-	}
-	return(is_equal);
+	return TRUE;
 }
 
 /*********************************************************************//**

storage/innobase/row/row0upd.cc

@@ -1116,10 +1116,6 @@ row_upd_build_difference_binary(
 	for purge/mvcc purpose) */
 	if (n_v_fld > 0) {
 		row_ext_t*	ext;
-		mem_heap_t*	v_heap = NULL;
-		byte*		record;
-		VCOL_STORAGE*	vcol_storage;
-
 		THD*		thd;
 
 		if (trx == NULL) {
@@ -1130,9 +1126,8 @@ row_upd_build_difference_binary(
 
 		ut_ad(!update->old_vrow);
 
-		innobase_allocate_row_for_vcol(thd, index, &v_heap,
-					       &mysql_table,
-					       &record, &vcol_storage);
+		ib_vcol_row vc(NULL);
+		uchar *record = vc.record(thd, index, &mysql_table);
 
 		for (i = 0; i < n_v_fld; i++) {
 			const dict_v_col_t*     col
@@ -1150,10 +1145,9 @@ row_upd_build_difference_binary(
 
 			dfield_t*	vfield = innobase_get_computed_value(
 				update->old_vrow, col, index,
-				&v_heap, heap, NULL, thd, mysql_table, record,
+				&vc.heap, heap, NULL, thd, mysql_table, record,
 				NULL, NULL, NULL);
 			if (vfield == NULL) {
-				if (v_heap) mem_heap_free(v_heap);
 				*error = DB_COMPUTE_VALUE_FAILED;
 				return(NULL);
 			}
@@ -1182,12 +1176,6 @@ row_upd_build_difference_binary(
 
 			}
 		}
-
-		if (v_heap) {
-			if (vcol_storage)
-				innobase_free_row_for_vcol(vcol_storage);
-			mem_heap_free(v_heap);
-		}
 	}
 
 	update->n_fields = n_diff;
@@ -2122,14 +2110,8 @@ row_upd_store_v_row(
 	THD*		thd,
 	TABLE*		mysql_table)
 {
-	mem_heap_t*	heap = NULL;
 	dict_index_t*	index = dict_table_get_first_index(node->table);
-        byte*           record= 0;
-	VCOL_STORAGE	*vcol_storage= 0;
-
-	if (!update)
-	  innobase_allocate_row_for_vcol(thd, index, &heap, &mysql_table,
-					 &record, &vcol_storage);
+	ib_vcol_row	vc(NULL);
 
 	for (ulint col_no = 0; col_no < dict_table_get_n_v_cols(node->table);
 	     col_no++) {
@@ -2179,24 +2161,19 @@ row_upd_store_v_row(
 						dfield_dup(dfield, node->heap);
 					}
 				} else {
+					uchar *record = vc.record(thd, index,
+								  &mysql_table);
 					/* Need to compute, this happens when
 					deleting row */
 					innobase_get_computed_value(
 						node->row, col, index,
-						&heap, node->heap, NULL,
-						thd, mysql_table, record, NULL,
-						NULL, NULL);
+						&vc.heap, node->heap,
+						NULL, thd, mysql_table,
+						record, NULL, NULL, NULL);
 				}
 			}
 		}
 	}
-
-	if (heap) {
-		if (vcol_storage)
-			innobase_free_row_for_vcol(vcol_storage);
-		mem_heap_free(heap);
-	}
-
 }
 
 /** Stores to the heap the row on which the node->pcur is positioned.

storage/innobase/row/row0vers.cc

@@ -455,11 +455,8 @@ row_vers_build_clust_v_col(
 	mem_heap_t*		heap,
 	purge_vcol_info_t*	vcol_info)
 {
-	mem_heap_t*	local_heap = NULL;
-	VCOL_STORAGE	*vcol_storage= NULL;
 	THD*		thd= current_thd;
 	TABLE*		maria_table= 0;
-	byte*		record= 0;
 
 	ut_ad(dict_index_has_virtual(index));
 	ut_ad(index->table == clust_index->table);
@@ -469,15 +466,12 @@ row_vers_build_clust_v_col(
 		maria_table = vcol_info->table();
 	}
 
-	innobase_allocate_row_for_vcol(thd, index,
-				       &local_heap,
-				       &maria_table,
-				       &record,
-				       &vcol_storage);
+	ib_vcol_row vc(NULL);
+	byte *record = vc.record(thd, index, &maria_table);
 
 	if (vcol_info && !vcol_info->table()) {
 		vcol_info->set_table(maria_table);
-		goto func_exit;
+		return;
 	}
 
 	for (ulint i = 0; i < dict_index_get_n_fields(index); i++) {
@@ -491,18 +485,11 @@ row_vers_build_clust_v_col(
 				ind_field->col);
 
 			innobase_get_computed_value(
-				row, col, clust_index, &local_heap,
+				row, col, clust_index, &vc.heap,
 				heap, NULL, thd, maria_table, record, NULL,
 				NULL, NULL);
 		}
 	}
-
-func_exit:
-	if (local_heap) {
-		if (vcol_storage)
-			innobase_free_row_for_vcol(vcol_storage);
-		mem_heap_free(local_heap);
-	}
 }
 
 /** Build latest virtual column data from undo log