From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Vladimir Davydov Subject: [PATCH v2] alter: fix rollback in case DDL and DML are used in the same transaction Date: Tue, 30 Jul 2019 19:26:15 +0300 Message-Id: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit To: tarantool-patches@freelists.org List-ID: A txn_stmt keeps a reference to the space it modifies. Memtx uses this space reference to revert the statement on error or voluntary rollback so the space must stay valid throughout the whole transaction. The problem is a DML statement may be followed by a DDL statement that modifies the target space in the same transaction. If we try to roll it back before running the rollback triggers installed by the DDL statement, it will access an invalid space object (e.g. missing an index), which will result in a crash. To fix this problem, let's run triggers installed by a statement right after rolling back the statement. Closes #4368 --- https://github.com/tarantool/tarantool/issues/4368 https://github.com/tarantool/tarantool/commits/dv/gh-4368-fix-mixed-ddl-dml-crash Changes in v2: - Instead of modifying MoveIndex and ModifyIndex operations so as to ensure that the old space stays valid until commit/rollback, run rollback triggers set by DDL statements synchronously with undoing the statements. This way by the time we get to undoing a DML request the target space will be reverted to the state it was when the DML request was executed. v1: https://www.freelists.org/post/tarantool-patches/PATCH-33-alter-fix-rollback-in-case-DDL-and-DML-are-used-in-the-same-transaction src/box/memtx_engine.c | 11 +------ src/box/txn.c | 57 +++++++++++++++++------------------ src/box/vy_tx.c | 3 +- test/box/transaction.result | 27 +++++++++++++++++ test/box/transaction.test.lua | 20 ++++++++++++ 5 files changed, 77 insertions(+), 41 deletions(-) diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c index 8bf90b60..59ad1682 100644 --- a/src/box/memtx_engine.c +++ b/src/box/memtx_engine.c @@ -369,15 +369,6 @@ memtx_engine_rollback_statement(struct engine *engine, struct txn *txn, tuple_unref(stmt->new_tuple); } -static void -memtx_engine_rollback(struct engine *engine, struct txn *txn) -{ - struct txn_stmt *stmt; - stailq_reverse(&txn->stmts); - stailq_foreach_entry(stmt, &txn->stmts, next) - memtx_engine_rollback_statement(engine, txn, stmt); -} - static int memtx_engine_bootstrap(struct engine *engine) { @@ -862,7 +853,7 @@ static const struct engine_vtab memtx_engine_vtab = { /* .prepare = */ generic_engine_prepare, /* .commit = */ generic_engine_commit, /* .rollback_statement = */ memtx_engine_rollback_statement, - /* .rollback = */ memtx_engine_rollback, + /* .rollback = */ generic_engine_rollback, /* .switch_to_ro = */ generic_engine_switch_to_ro, /* .bootstrap = */ memtx_engine_bootstrap, /* .begin_initial_recovery = */ memtx_engine_begin_initial_recovery, diff --git a/src/box/txn.c b/src/box/txn.c index 9599284b..53ebfc05 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -116,30 +116,36 @@ txn_stmt_unref_tuples(struct txn_stmt *stmt) tuple_unref(stmt->new_tuple); } +/* + * Undo changes done by a statement and run the corresponding + * rollback triggers. + * + * Note, a trigger set by a particular statement must be run right + * after the statement is rolled back, because rollback triggers + * installed by DDL statements restore the schema cache, which is + * necessary to roll back previous statements. For example, to roll + * back a DML statement applied to a space whose index is dropped + * later in the same transaction, we must restore the dropped index + * first. + */ +static void +txn_rollback_one_stmt(struct txn *txn, struct txn_stmt *stmt) +{ + if (txn->engine != NULL && stmt->space != NULL) + engine_rollback_statement(txn->engine, txn, stmt); + if (stmt->has_triggers) + txn_run_rollback_triggers(txn, &stmt->on_rollback); +} + static void txn_rollback_to_svp(struct txn *txn, struct stailq_entry *svp) { - /* - * Undo changes done to the database by the rolled back - * statements and collect corresponding rollback triggers. - * Don't release the tuples as rollback triggers might - * still need them. - */ struct txn_stmt *stmt; struct stailq rollback; - RLIST_HEAD(on_rollback); stailq_cut_tail(&txn->stmts, svp, &rollback); stailq_reverse(&rollback); stailq_foreach_entry(stmt, &rollback, next) { - /* - * Note the statement list is reversed hence - * we must append triggers to the tail so as - * to preserve the rollback order. - */ - if (stmt->has_triggers) - rlist_splice_tail(&on_rollback, &stmt->on_rollback); - if (txn->engine != NULL && stmt->space != NULL) - engine_rollback_statement(txn->engine, txn, stmt); + txn_rollback_one_stmt(txn, stmt); if (stmt->row != NULL && stmt->row->replica_id == 0) { assert(txn->n_new_rows > 0); txn->n_new_rows--; @@ -150,15 +156,10 @@ txn_rollback_to_svp(struct txn *txn, struct stailq_entry *svp) assert(txn->n_applier_rows > 0); txn->n_applier_rows--; } + txn_stmt_unref_tuples(stmt); stmt->space = NULL; stmt->row = NULL; } - /* Run rollback triggers installed after the savepoint. */ - txn_run_rollback_triggers(txn, &on_rollback); - - /* Release the tuples. */ - stailq_foreach_entry(stmt, &rollback, next) - txn_stmt_unref_tuples(stmt); } /* @@ -420,6 +421,10 @@ txn_complete(struct txn *txn) */ if (txn->signature < 0) { /* Undo the transaction. */ + struct txn_stmt *stmt; + stailq_reverse(&txn->stmts); + stailq_foreach_entry(stmt, &txn->stmts, next) + txn_rollback_one_stmt(txn, stmt); if (txn->engine) engine_rollback(txn->engine, txn); if (txn_has_flag(txn, TXN_HAS_TRIGGERS)) @@ -494,7 +499,6 @@ txn_write_to_wal(struct txn *txn) if (stmt->has_triggers) { txn_init_triggers(txn); rlist_splice(&txn->on_commit, &stmt->on_commit); - rlist_splice(&txn->on_rollback, &stmt->on_rollback); } if (stmt->row == NULL) continue; /* A read (e.g. select) request */ @@ -619,13 +623,6 @@ txn_rollback(struct txn *txn) trigger_clear(&txn->fiber_on_stop); if (!txn_has_flag(txn, TXN_CAN_YIELD)) trigger_clear(&txn->fiber_on_yield); - struct txn_stmt *stmt; - stailq_foreach_entry(stmt, &txn->stmts, next) { - if (stmt->has_triggers) { - txn_init_triggers(txn); - rlist_splice(&txn->on_rollback, &stmt->on_rollback); - } - } txn->signature = -1; txn_complete(txn); fiber_set_txn(fiber(), NULL); diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c index d0e92068..9b300fde 100644 --- a/src/box/vy_tx.c +++ b/src/box/vy_tx.c @@ -908,7 +908,8 @@ vy_tx_begin_statement(struct vy_tx *tx, struct space *space, void **savepoint) void vy_tx_rollback_statement(struct vy_tx *tx, void *svp) { - if (tx->state == VINYL_TX_ABORT) + if (tx->state == VINYL_TX_ABORT || + tx->state == VINYL_TX_COMMIT) return; assert(tx->state == VINYL_TX_READY); diff --git a/test/box/transaction.result b/test/box/transaction.result index 1ed649d2..718d7422 100644 --- a/test/box/transaction.result +++ b/test/box/transaction.result @@ -817,3 +817,30 @@ box.begin() box.execute([[CREATE TABLE test(id INTEGER PRIMARY KEY AUTOINCREMENT box.rollback() --- ... +-- +-- gh-4368: transaction rollback leads to a crash if DDL and DML statements +-- are mixed in the same transaction. +-- +test_run:cmd("setopt delimiter ';'") +--- +- true +... +for i = 1, 100 do + box.begin() + s = box.schema.space.create('test') + s:create_index('pk') + s:create_index('sk', {unique = true, parts = {2, 'unsigned'}}) + s:insert{1, 1} + s.index.sk:alter{unique = false} + s:insert{2, 1} + s.index.sk:drop() + s:delete{2} + box.rollback() + fiber.sleep(0) +end; +--- +... +test_run:cmd("setopt delimiter ''"); +--- +- true +... diff --git a/test/box/transaction.test.lua b/test/box/transaction.test.lua index a0014c9f..e057483f 100644 --- a/test/box/transaction.test.lua +++ b/test/box/transaction.test.lua @@ -438,3 +438,23 @@ check1, check2, check3, check4 -- box.begin() box.execute([[CREATE TABLE test(id INTEGER PRIMARY KEY AUTOINCREMENT)]]) fiber.yield() box.rollback() + +-- +-- gh-4368: transaction rollback leads to a crash if DDL and DML statements +-- are mixed in the same transaction. +-- +test_run:cmd("setopt delimiter ';'") +for i = 1, 100 do + box.begin() + s = box.schema.space.create('test') + s:create_index('pk') + s:create_index('sk', {unique = true, parts = {2, 'unsigned'}}) + s:insert{1, 1} + s.index.sk:alter{unique = false} + s:insert{2, 1} + s.index.sk:drop() + s:delete{2} + box.rollback() + fiber.sleep(0) +end; +test_run:cmd("setopt delimiter ''"); -- 2.20.1