Skip to content

Commit

Permalink
MDEV-24786: row_upd_clust_step() skips mtr_t::commit() on virtual col…
Browse files Browse the repository at this point in the history 
…umn 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 97db6c1 (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.
  • Loading branch information
@dr-m
dr-m committed Mar 26, 2021
1 parent da26e2e commit a6d66fe
Showing 1 changed file with 46 additions and 80 deletions.
126 changes: 46 additions & 80 deletions storage/innobase/row/row0upd.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,39 +126,37 @@ 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
= std::find_if(table->referenced_set.begin(),
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
Expand Down Expand Up @@ -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;
Expand All @@ -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 */
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}

Expand All @@ -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;
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -2954,7 +2949,6 @@ row_upd_clust_rec(
}
}

mtr_commit(mtr);
func_exit:
if (heap) {
mem_heap_free(heap);
Expand All @@ -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);

Expand All @@ -3005,16 +2999,15 @@ 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
locks, because we assume that we have an x-lock on the record */

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);

Expand Down Expand Up @@ -3051,8 +3044,6 @@ row_upd_del_mark_clust_rec(
#endif /* WITH_WSREP */
}

mtr_commit(mtr);

return(err);
}

Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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;
}
}

Expand All @@ -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;
}
}
Expand All @@ -3180,22 +3157,14 @@ 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,
#ifdef WITH_WSREP
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
Expand All @@ -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;
}
Expand All @@ -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;
}

/***********************************************************//**
Expand Down

0 comments on commit a6d66fe

Please sign in to comment.