diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
index 8bf90b6012c5f4324301eaf087c5b55387995cb7..59ad16823681c70b058bd3770553258857aaf060 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 9599284bc578cd4e7d36022f885261777c434dac..53ebfc05354218221059c493362078bf26891732 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 d0e9206880e52dbc7de2c5711ee0b037c4d0027d..9b300fde692d5028129eb4ffe59ec201b0c96461 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 1ed649d26ff9f70a0ad5fc336a45366923fe70fa..718d74228780fc698518298bc47b12b61a71ac59 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 a0014c9f5e5fc2358df574c21037bad6835de105..e057483f696de0f36112fc1487f80ad0905b2014 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 ''");