[PATCH v2] alter: fix rollback in case DDL and DML are used in the same transaction
Vladimir Davydov
vdavydov.dev at gmail.com
Tue Jul 30 19:26:15 MSK 2019
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
More information about the Tarantool-patches
mailing list