[PATCH v2] alter: fix rollback in case DDL and DML are used in the same transaction

[PATCH v2] alter: fix rollback in case DDL and DML are used in the same transaction

[Vladimir Davydov]

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

[tarantool-patches] Re: [PATCH v2] alter: fix rollback in case DDL and DML are used in the same transaction

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/30 20:33]:
> 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

LGTM.

The final fix could actually be to make Lua commit/rollback
triggers append to statemenet list, not transaction list, and
delete transaction list altogether.


 

-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH v2] alter: fix rollback in case DDL and DML are used in the same transaction

[Vladimir Davydov]

Pushed to master.