[PATCH 1.7 3/4] txn: fix rollback of sub-statements
Vladimir Davydov
vdavydov.dev at gmail.com
Tue Jan 23 13:06:13 MSK 2018
If space.on_replace callback fails (throws), we must rollback all
statements inserted by the callback before failing and the statement
that triggered the callback while currently we only rollback the last
statement on txn->stmts list. To fix this, let's remember the position
to rollback to in case of failure for each sub statement, similarly to
how we do with savepoints.
Note, there's a comment to txn_rollback_stmt() that says that it doesn't
remove the last statement from the txn->stmts list, just clears it:
/**
* Void all effects of the statement, but
* keep it in the list - to maintain
* limit on the number of statements in a
* transaction.
*/
void
txn_rollback_stmt()
It isn't going to hold after this patch, because this patch reuses the
savepoint code to rollback statements on failure. Anyway, I haven't
managed to figure out why we would ever need to keep statements on the
list after rollback. The comment is clearly misleading as we don't have
any limits on the number of statements, and even if we had, a statement
counter would suffice. I guess the real reason why we don't delete
statements from the list is that it is simply impossible to do in case
of a singly linked list like stailq, but now it isn't going to be a
problem. So remove the comment and the test case of engine/savepoint
which relies on it.
Needed for #2993
Closes #3020
---
src/box/txn.c | 61 +++++++++++----------------
src/box/txn.h | 14 +++++++
test/box/on_replace.result | 95 ++++++++++++++++++++++++++++++++++++++++++
test/box/on_replace.test.lua | 51 +++++++++++++++++++++++
test/engine/savepoint.result | 29 -------------
test/engine/savepoint.test.lua | 16 -------
6 files changed, 185 insertions(+), 81 deletions(-)
diff --git a/src/box/txn.c b/src/box/txn.c
index 5d5b579b..d017b967 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -35,14 +35,6 @@
#include <fiber.h>
#include "xrow.h"
-enum {
- /**
- * Maximum recursion depth for on_replace triggers.
- * Large numbers may corrupt C stack.
- */
- TXN_SUB_STMT_MAX = 3
-};
-
double too_long_threshold;
static inline void
@@ -98,11 +90,32 @@ txn_stmt_new(struct txn *txn)
stmt->engine_savepoint = NULL;
stmt->row = NULL;
+ /* Set the savepoint for statement rollback. */
+ txn->sub_stmt_begin[txn->in_sub_stmt] = stailq_last(&txn->stmts);
+ txn->in_sub_stmt++;
+
stailq_add_tail_entry(&txn->stmts, stmt, next);
- ++txn->in_sub_stmt;
return stmt;
}
+static void
+txn_rollback_to_svp(struct txn *txn, struct stailq_entry *svp)
+{
+ struct txn_stmt *stmt;
+ struct stailq rollback;
+ stailq_cut_tail(&txn->stmts, svp, &rollback);
+ stailq_reverse(&rollback);
+ stailq_foreach_entry(stmt, &rollback, next) {
+ engine_rollback_statement(txn->engine, txn, stmt);
+ if (stmt->row != NULL) {
+ assert(txn->n_rows > 0);
+ txn->n_rows--;
+ stmt->row = NULL;
+ }
+ stmt->space = NULL;
+ }
+}
+
struct txn *
txn_begin(bool is_autocommit)
{
@@ -309,12 +322,6 @@ fail:
return -1;
}
-/**
- * Void all effects of the statement, but
- * keep it in the list - to maintain
- * limit on the number of statements in a
- * transaction.
- */
void
txn_rollback_stmt()
{
@@ -325,15 +332,8 @@ txn_rollback_stmt()
return txn_rollback();
if (txn->in_sub_stmt == 0)
return;
- struct txn_stmt *stmt = stailq_last_entry(&txn->stmts, struct txn_stmt,
- next);
- engine_rollback_statement(txn->engine, txn, stmt);
- if (stmt->row != NULL) {
- stmt->row = NULL;
- --txn->n_rows;
- assert(txn->n_rows >= 0);
- }
- --txn->in_sub_stmt;
+ txn->in_sub_stmt--;
+ txn_rollback_to_svp(txn, txn->sub_stmt_begin[txn->in_sub_stmt]);
}
void
@@ -483,17 +483,6 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
diag_set(ClientError, ER_NO_SUCH_SAVEPOINT);
return -1;
}
- struct stailq rollback_stmts;
- stailq_cut_tail(&txn->stmts, svp->stmt, &rollback_stmts);
- stailq_reverse(&rollback_stmts);
- stailq_foreach_entry(stmt, &rollback_stmts, next) {
- engine_rollback_statement(txn->engine, txn, stmt);
- if (stmt->row != NULL) {
- stmt->row = NULL;
- --txn->n_rows;
- assert(txn->n_rows >= 0);
- }
- stmt->space = NULL;
- }
+ txn_rollback_to_svp(txn, svp->stmt);
return 0;
}
diff --git a/src/box/txn.h b/src/box/txn.h
index f9f25d5d..f8d3487f 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -49,6 +49,14 @@ struct space;
struct tuple;
struct xrow_header;
+enum {
+ /**
+ * Maximum recursion depth for on_replace triggers.
+ * Large numbers may corrupt C stack.
+ */
+ TXN_SUB_STMT_MAX = 3
+};
+
/**
* A single statement of a multi-statement
* transaction: undo and redo info.
@@ -110,6 +118,12 @@ struct txn {
bool has_triggers;
/** The number of active nested statement-level transactions. */
int in_sub_stmt;
+ /**
+ * First statement at each statement-level.
+ * Needed to rollback sub statements.
+ */
+ struct stailq_entry *sub_stmt_begin[TXN_SUB_STMT_MAX];
+ /** LSN of this transaction when written to WAL. */
int64_t signature;
/** Engine involved in multi-statement transaction. */
struct engine *engine;
diff --git a/test/box/on_replace.result b/test/box/on_replace.result
index 7b6f8f38..e05791ff 100644
--- a/test/box/on_replace.result
+++ b/test/box/on_replace.result
@@ -529,3 +529,98 @@ s:select()
s:drop()
---
...
+--
+-- gh-3020: sub-statement rollback
+--
+s1 = box.schema.space.create('test1')
+---
+...
+_ = s1:create_index('pk')
+---
+...
+s2 = box.schema.space.create('test2')
+---
+...
+_ = s2:create_index('pk')
+---
+...
+s3 = box.schema.space.create('test3')
+---
+...
+_ = s3:create_index('pk')
+---
+...
+test_run:cmd("setopt delimiter ';'");
+---
+- true
+...
+x1 = 1;
+---
+...
+x2 = 1;
+---
+...
+_ = s1:on_replace(function(old, new)
+ for i = 1, 3 do
+ s2:insert{x1}
+ x1 = x1 + 1
+ end
+ if new[2] == 'fail' then
+ error('fail')
+ end
+ pcall(s2.insert, s2, {123, 'fail'})
+end);
+---
+...
+_ = s2:on_replace(function(old, new)
+ for i = 1, 3 do
+ s3:insert{x2}
+ x2 = x2 + 1
+ end
+ if new[2] == 'fail' then
+ error('fail')
+ end
+end);
+---
+...
+box.begin()
+s1:insert{1}
+pcall(s1.insert, s1, {123, 'fail'})
+box.commit();
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+s1:select()
+---
+- - [1]
+...
+s2:select()
+---
+- - [1]
+ - [2]
+ - [3]
+...
+s3:select()
+---
+- - [1]
+ - [2]
+ - [3]
+ - [4]
+ - [5]
+ - [6]
+ - [7]
+ - [8]
+ - [9]
+...
+s1:drop()
+---
+...
+s2:drop()
+---
+...
+s3:drop()
+---
+...
diff --git a/test/box/on_replace.test.lua b/test/box/on_replace.test.lua
index ea42b42d..906fd99b 100644
--- a/test/box/on_replace.test.lua
+++ b/test/box/on_replace.test.lua
@@ -199,3 +199,54 @@ t = s:on_replace(function () s.index.pk:rename('newname') end, t)
s:replace({9, 10})
s:select()
s:drop()
+
+--
+-- gh-3020: sub-statement rollback
+--
+s1 = box.schema.space.create('test1')
+_ = s1:create_index('pk')
+s2 = box.schema.space.create('test2')
+_ = s2:create_index('pk')
+s3 = box.schema.space.create('test3')
+_ = s3:create_index('pk')
+
+test_run:cmd("setopt delimiter ';'");
+
+x1 = 1;
+x2 = 1;
+
+_ = s1:on_replace(function(old, new)
+ for i = 1, 3 do
+ s2:insert{x1}
+ x1 = x1 + 1
+ end
+ if new[2] == 'fail' then
+ error('fail')
+ end
+ pcall(s2.insert, s2, {123, 'fail'})
+end);
+
+_ = s2:on_replace(function(old, new)
+ for i = 1, 3 do
+ s3:insert{x2}
+ x2 = x2 + 1
+ end
+ if new[2] == 'fail' then
+ error('fail')
+ end
+end);
+
+box.begin()
+s1:insert{1}
+pcall(s1.insert, s1, {123, 'fail'})
+box.commit();
+
+test_run:cmd("setopt delimiter ''");
+
+s1:select()
+s2:select()
+s3:select()
+
+s1:drop()
+s2:drop()
+s3:drop()
diff --git a/test/engine/savepoint.result b/test/engine/savepoint.result
index 1ef159ce..9d238ecc 100644
--- a/test/engine/savepoint.result
+++ b/test/engine/savepoint.result
@@ -501,35 +501,6 @@ s:select{}
s:truncate()
---
...
--- Savepoints on different statements, but on a same state.
-test_run:cmd("setopt delimiter ';'")
----
-- true
-...
-box.begin()
-s:replace{1}
-s1 = box.savepoint()
-pcall(s.replace, s, {'2'})
-s2 = box.savepoint()
-s:replace{3}
-box.rollback_to_savepoint(s1)
-ok1, errmsg1 = pcall(box.rollback_to_savepoint, s2)
-box.commit()
-test_run:cmd("setopt delimiter ''");
----
-...
-s:select{}
----
-- - [1]
-...
-ok1, errmsg1
----
-- false
-- 'Can not rollback to savepoint: the savepoint does not exist'
-...
-s:truncate()
----
-...
-- Several savepoints on a same statement.
test_run:cmd("setopt delimiter ';'")
---
diff --git a/test/engine/savepoint.test.lua b/test/engine/savepoint.test.lua
index 11df0d65..de8f2977 100644
--- a/test/engine/savepoint.test.lua
+++ b/test/engine/savepoint.test.lua
@@ -247,22 +247,6 @@ test_run:cmd("setopt delimiter ''");
s:select{}
s:truncate()
--- Savepoints on different statements, but on a same state.
-test_run:cmd("setopt delimiter ';'")
-box.begin()
-s:replace{1}
-s1 = box.savepoint()
-pcall(s.replace, s, {'2'})
-s2 = box.savepoint()
-s:replace{3}
-box.rollback_to_savepoint(s1)
-ok1, errmsg1 = pcall(box.rollback_to_savepoint, s2)
-box.commit()
-test_run:cmd("setopt delimiter ''");
-s:select{}
-ok1, errmsg1
-s:truncate()
-
-- Several savepoints on a same statement.
test_run:cmd("setopt delimiter ';'")
box.begin()
--
2.11.0
More information about the Tarantool-patches
mailing list