From 35a486884c94055e6d3f59abdf910aa46c150a34 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Tue, 30 Jul 2019 16:39:16 +0300
Subject: [PATCH] txn: fix rollback in case DDL and DML are used in the same
transaction
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
---
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 8bf90b6012..59ad168236 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 9599284bc5..53ebfc0535 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 d0e9206880..9b300fde69 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 1ed649d26f..718d742287 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 a0014c9f5e..e057483f69 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 ''");
--
GitLab