MDEV-24786: row_upd_clust_step() skips mtr_t::commit() on virtual column error

The function row_upd_clust_step() is invoking several static functions,
some of which used to commit the mini-transaction in some cases.
If innobase_get_computed_value() would fail due to some reason,
we would fail to invoke mtr_t::commit() and release buffer pool
page latches. This would likely lead to a hanging server later.

This regression was introduced in
commit 97db6c15 (MDEV-20618).

row_upd_index_is_referenced(), row_upd_sec_index_entry(),
row_upd_sec_index_entry(): Cleanup: Replace some ibool with bool.

row_upd_clust_rec_by_insert(), row_upd_clust_rec(): Guarantee that
the mini-transaction will always remain in active state.

row_upd_del_mark_clust_rec(): Guarantee that
the mini-transaction will always remain in active state.
This fixes one "leak" of mini-transaction on DB_COMPUTE_VALUE_FAILED.

row_upd_clust_step(): Use only one return path, which will always
invoke mtr.commit(). After a failed row_upd_store_row() call, we
will no longer "leak" the mini-transaction.

This fix was verified by RQG on 10.6 (depending on MDEV-371 that
was introduced in 10.4). Unfortunately, it is challenging to
create a regression test for this, and a test case could soon become
invalid as more bugs in virtual column evaluation are fixed.

storage/innobase/row/row0upd.cc

@@ -126,25 +126,23 @@ NOTE that since we do not hold dict_operation_lock when leaving the
 function, it may be that the referencing table has been dropped when
 we leave this function: this function is only for heuristic use!
 
-@return TRUE if referenced */
+@return true if referenced */
 static
-ibool
+bool
 row_upd_index_is_referenced(
 /*========================*/
 	dict_index_t*	index,	/*!< in: index */
 	trx_t*		trx)	/*!< in: transaction */
 {
 	dict_table_t*	table		= index->table;
-	ibool		froze_data_dict	= FALSE;
-	ibool		is_referenced	= FALSE;
 
 	if (table->referenced_set.empty()) {
-		return(FALSE);
+		return false;
 	}
 
-	if (trx->dict_operation_lock_mode == 0) {
+	const bool froze_data_dict = !trx->dict_operation_lock_mode;
+	if (froze_data_dict) {
 		row_mysql_freeze_data_dictionary(trx);
-		froze_data_dict = TRUE;
 	}
 
 	dict_foreign_set::iterator	it
@@ -152,13 +150,13 @@ row_upd_index_is_referenced(
 			       table->referenced_set.end(),
 			       dict_foreign_with_index(index));
 
-	is_referenced = (it != table->referenced_set.end());
+	const bool is_referenced = (it != table->referenced_set.end());
 
 	if (froze_data_dict) {
 		row_mysql_unfreeze_data_dictionary(trx);
 	}
 
-	return(is_referenced);
+	return is_referenced;
 }
 
 #ifdef WITH_WSREP
@@ -2278,7 +2276,6 @@ row_upd_sec_index_entry(
 	dtuple_t*		entry;
 	dict_index_t*		index;
 	btr_cur_t*		btr_cur;
-	ibool			referenced;
 	dberr_t			err	= DB_SUCCESS;
 	trx_t*			trx	= thr_get_trx(thr);
 	ulint			mode;
@@ -2289,7 +2286,7 @@ row_upd_sec_index_entry(
 
 	index = node->index;
 
-	referenced = row_upd_index_is_referenced(index, trx);
+	const bool referenced = row_upd_index_is_referenced(index, trx);
 #ifdef WITH_WSREP
 	bool foreign = wsrep_row_upd_index_is_foreign(index, trx);
 #endif /* WITH_WSREP */
@@ -2690,12 +2687,13 @@ row_upd_clust_rec_by_insert(
 	upd_node_t*	node,	/*!< in/out: row update node */
 	dict_index_t*	index,	/*!< in: clustered index of the record */
 	que_thr_t*	thr,	/*!< in: query thread */
-	ibool		referenced,/*!< in: TRUE if index may be referenced in
+	bool		referenced,/*!< in: whether index may be referenced in
 				a foreign key constraint */
 #ifdef WITH_WSREP
 	bool		foreign,/*!< in: whether this is a foreign key */
 #endif
-	mtr_t*		mtr)	/*!< in/out: mtr; gets committed here */
+	mtr_t*		mtr)	/*!< in/out: mini-transaction,
+				may be committed and restarted */
 {
 	mem_heap_t*	heap;
 	btr_pcur_t*	pcur;
@@ -2760,10 +2758,7 @@ row_upd_clust_rec_by_insert(
 			btr_cur_get_block(btr_cur), rec, index, offsets,
 			thr, node->row, mtr);
 		if (err != DB_SUCCESS) {
-err_exit:
-			mtr_commit(mtr);
-			mem_heap_free(heap);
-			return(err);
+			goto err_exit;
 		}
 
 		/* If the the new row inherits externally stored
@@ -2822,14 +2817,14 @@ row_upd_clust_rec_by_insert(
 		}
 	}
 
-	mtr_commit(mtr);
+	mtr->commit();
+	mtr->start();
 
+	node->state = UPD_NODE_INSERT_CLUSTERED;
 	err = row_ins_clust_index_entry(
 		index, entry, thr, dtuple_get_n_ext(entry));
-	node->state = UPD_NODE_INSERT_CLUSTERED;
-
+err_exit:
 	mem_heap_free(heap);
-
 	return(err);
 }
 
@@ -2849,7 +2844,8 @@ row_upd_clust_rec(
 	mem_heap_t**	offsets_heap,
 				/*!< in/out: memory heap, can be emptied */
 	que_thr_t*	thr,	/*!< in: query thread */
-	mtr_t*		mtr)	/*!< in: mtr; gets committed here */
+	mtr_t*		mtr)	/*!< in,out: mini-transaction; may be
+				committed and restarted here */
 {
 	mem_heap_t*	heap		= NULL;
 	big_rec_t*	big_rec		= NULL;
@@ -2895,16 +2891,15 @@ row_upd_clust_rec(
 		goto success;
 	}
 
-	mtr_commit(mtr);
-
 	if (buf_LRU_buf_pool_running_out()) {
-
 		err = DB_LOCK_TABLE_FULL;
 		goto func_exit;
 	}
+
 	/* We may have to modify the tree structure: do a pessimistic descent
 	down the index tree */
 
+	mtr->commit();
 	mtr->start();
 
 	if (index->table->is_temporary()) {
@@ -2954,7 +2949,6 @@ row_upd_clust_rec(
 		}
 	}
 
-	mtr_commit(mtr);
 func_exit:
 	if (heap) {
 		mem_heap_free(heap);
@@ -2979,17 +2973,17 @@ row_upd_del_mark_clust_rec(
 	rec_offs*	offsets,/*!< in/out: rec_get_offsets() for the
 				record under the cursor */
 	que_thr_t*	thr,	/*!< in: query thread */
-	ibool		referenced,
-				/*!< in: TRUE if index may be referenced in
+	bool		referenced,
+				/*!< in: whether index may be referenced in
 				a foreign key constraint */
 #ifdef WITH_WSREP
 	bool		foreign,/*!< in: whether this is a foreign key */
 #endif
-	mtr_t*		mtr)	/*!< in: mtr; gets committed here */
+	mtr_t*		mtr)	/*!< in,out: mini-transaction;
+				will be committed and restarted */
 {
 	btr_pcur_t*	pcur;
 	btr_cur_t*	btr_cur;
-	dberr_t		err;
 	rec_t*		rec;
 	trx_t*		trx = thr_get_trx(thr);
 
@@ -3005,8 +2999,7 @@ row_upd_del_mark_clust_rec(
 	if (!row_upd_store_row(node, trx->mysql_thd,
 			  thr->prebuilt  && thr->prebuilt->table == node->table
 			  ? thr->prebuilt->m_mysql_table : NULL)) {
-		err = DB_COMPUTE_VALUE_FAILED;
-		return err;
+		return DB_COMPUTE_VALUE_FAILED;
 	}
 
 	/* Mark the clustered index record deleted; we do not have to check
@@ -3014,7 +3007,7 @@ row_upd_del_mark_clust_rec(
 
 	rec = btr_cur_get_rec(btr_cur);
 
-	err = btr_cur_del_mark_set_clust_rec(
+	dberr_t err = btr_cur_del_mark_set_clust_rec(
 		btr_cur_get_block(btr_cur), rec,
 		index, offsets, thr, node->row, mtr);
 
@@ -3051,8 +3044,6 @@ row_upd_del_mark_clust_rec(
 #endif /* WITH_WSREP */
 	}
 
-	mtr_commit(mtr);
-
 	return(err);
 }
 
@@ -3069,22 +3060,19 @@ row_upd_clust_step(
 {
 	dict_index_t*	index;
 	btr_pcur_t*	pcur;
-	ibool		success;
 	dberr_t		err;
 	mtr_t		mtr;
 	rec_t*		rec;
 	mem_heap_t*	heap	= NULL;
 	rec_offs	offsets_[REC_OFFS_NORMAL_SIZE];
 	rec_offs*	offsets;
-	ibool		referenced;
 	trx_t*		trx = thr_get_trx(thr);
 
 	rec_offs_init(offsets_);
 
 	index = dict_table_get_first_index(node->table);
 
-	referenced = row_upd_index_is_referenced(index, trx);
-
+	const bool referenced = row_upd_index_is_referenced(index, trx);
 #ifdef WITH_WSREP
 	const bool foreign = wsrep_row_upd_index_is_foreign(index, trx);
 #endif
@@ -3125,14 +3113,9 @@ row_upd_clust_step(
 		mode = BTR_MODIFY_LEAF;
 	}
 
-	success = btr_pcur_restore_position(mode, pcur, &mtr);
-
-	if (!success) {
+	if (!btr_pcur_restore_position(mode, pcur, &mtr)) {
 		err = DB_RECORD_NOT_FOUND;
-
-		mtr_commit(&mtr);
-
-		return(err);
+		goto exit_func;
 	}
 
 	/* If this is a row in SYS_INDEXES table of the data dictionary,
@@ -3151,14 +3134,9 @@ row_upd_clust_step(
 		mtr.start();
 		mtr.set_named_space(index->space);
 
-		success = btr_pcur_restore_position(BTR_MODIFY_LEAF, pcur,
-						    &mtr);
-		if (!success) {
+		if (!btr_pcur_restore_position(BTR_MODIFY_LEAF, pcur, &mtr)) {
 			err = DB_ERROR;
-
-			mtr.commit();
-
-			return(err);
+			goto exit_func;
 		}
 	}
 
@@ -3171,7 +3149,6 @@ row_upd_clust_step(
 			0, btr_pcur_get_block(pcur),
 			rec, index, offsets, thr);
 		if (err != DB_SUCCESS) {
-			mtr.commit();
 			goto exit_func;
 		}
 	}
@@ -3180,8 +3157,6 @@ row_upd_clust_step(
 				      btr_pcur_get_block(pcur),
 				      page_rec_get_heap_no(rec)));
 
-	/* NOTE: the following function calls will also commit mtr */
-
 	if (node->is_delete) {
 		err = row_upd_del_mark_clust_rec(
 			node, index, offsets, thr, referenced,
@@ -3189,13 +3164,7 @@ row_upd_clust_step(
 			foreign,
 #endif
 			&mtr);
-
-		if (err == DB_SUCCESS) {
-			node->state = UPD_NODE_UPDATE_ALL_SEC;
-			node->index = dict_table_get_next_index(index);
-		}
-
-		goto exit_func;
+		goto all_done;
 	}
 
 	/* If the update is made for MySQL, we already have the update vector
@@ -3210,14 +3179,13 @@ row_upd_clust_step(
 	}
 
 	if (node->cmpl_info & UPD_NODE_NO_ORD_CHANGE) {
-
 		err = row_upd_clust_rec(
 			flags, node, index, offsets, &heap, thr, &mtr);
 		goto exit_func;
 	}
 
-	if(!row_upd_store_row(node, trx->mysql_thd,
-			thr->prebuilt ? thr->prebuilt->m_mysql_table : NULL)) {
+	if (!row_upd_store_row(node, trx->mysql_thd, thr->prebuilt
+			       ? thr->prebuilt->m_mysql_table : NULL)) {
 		err = DB_COMPUTE_VALUE_FAILED;
 		goto exit_func;
 	}
@@ -3242,31 +3210,29 @@ row_upd_clust_step(
 			foreign,
 #endif
 			&mtr);
-		if (err != DB_SUCCESS) {
-
-			goto exit_func;
+all_done:
+		if (err == DB_SUCCESS) {
+			node->state = UPD_NODE_UPDATE_ALL_SEC;
+success:
+			node->index = dict_table_get_next_index(index);
 		}
-
-		node->state = UPD_NODE_UPDATE_ALL_SEC;
 	} else {
 		err = row_upd_clust_rec(
 			flags, node, index, offsets, &heap, thr, &mtr);
 
-		if (err != DB_SUCCESS) {
-
-			goto exit_func;
+		if (err == DB_SUCCESS) {
+			ut_ad(!node->is_delete);
+			node->state = UPD_NODE_UPDATE_SOME_SEC;
+			goto success;
 		}
-
-		node->state = UPD_NODE_UPDATE_SOME_SEC;
 	}
 
-	node->index = dict_table_get_next_index(index);
-
 exit_func:
-	if (heap) {
+	mtr.commit();
+	if (UNIV_LIKELY_NULL(heap)) {
 		mem_heap_free(heap);
 	}
-	return(err);
+	return err;
 }
 
 /***********************************************************//**