From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Vladimir Davydov Subject: [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint Date: Fri, 19 Jul 2019 21:08:42 +0300 Message-Id: In-Reply-To: References: In-Reply-To: References: To: tarantool-patches@freelists.org List-ID: When reverting to a savepoint inside a DDL transaction, apart from undoing changes done by the DDL statements to the system spaces, we also have to - Run rollback triggers installed after the savepoint was set, because otherwise changes done to the schema by DDL won't be undone. - Remove commit triggers installed after the savepoint, because they are not relevant anymore, apparently. To achieve that, let's remember not only the last transaction statement at the time when a savepoint is created, but also the state of commit and rollback triggers. Since in contrast to txn statements, commit and rollback triggers can actually be deleted from the list, we create dummy triggers per each savepoint and use them as markers in the trigger lists. We don't however create dummy triggers if no commit/rollback triggers is installed, because in that case we would simply need to clear the trigger lists. While we are at it, let's also make sure that a rollback trigger doesn't an already freed tuple. To do that, we release statement tuples after running rollback triggers, not before as we used to. Closes #4364 Closes #4365 --- src/box/txn.c | 74 ++++++++++++++++++++++++++++++++++++++----- src/box/txn.h | 11 +++++++ test/box/transaction.result | 39 +++++++++++++++++++++++ test/box/transaction.test.lua | 23 ++++++++++++++ 4 files changed, 139 insertions(+), 8 deletions(-) diff --git a/src/box/txn.c b/src/box/txn.c index 3f0017f8..47afcaa0 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -46,20 +46,33 @@ txn_on_stop(struct trigger *trigger, void *event); static void txn_on_yield(struct trigger *trigger, void *event); +static void +txn_run_triggers(struct txn *txn, struct rlist *trigger); + static inline void fiber_set_txn(struct fiber *fiber, struct txn *txn) { fiber->storage.txn = txn; } +static void +dummy_trigger_cb(struct trigger *trigger, void *event) +{ + (void)trigger; + (void)event; +} + /** * A savepoint that points to the beginning of a transaction. * Use it to rollback all statements of any transaction. */ static struct txn_savepoint zero_svp = { /* .in_sub_stmt = */ 0, + /* .has_triggers = */ false, /* .stmt = */ NULL, /* .fk_deferred_count = */ 0, + /* .on_commit = */ {RLIST_LINK_INITIALIZER, 0, NULL, NULL}, + /* .on_rollback = */ {RLIST_LINK_INITIALIZER, 0, NULL, NULL}, }; /** @@ -71,10 +84,31 @@ txn_create_svp(struct txn *txn, struct txn_savepoint *svp) { svp->in_sub_stmt = txn->in_sub_stmt; svp->stmt = stailq_last(&txn->stmts); + svp->has_triggers = txn->has_triggers; + if (svp->has_triggers) { + trigger_create(&svp->on_commit, dummy_trigger_cb, NULL, NULL); + trigger_add(&txn->on_commit, &svp->on_commit); + trigger_create(&svp->on_rollback, dummy_trigger_cb, NULL, NULL); + trigger_add(&txn->on_rollback, &svp->on_rollback); + } if (txn->psql_txn != NULL) svp->fk_deferred_count = txn->psql_txn->fk_deferred_count; } +/** + * Destroy a savepoint that isn't going to be used anymore. + * This function clears dummy commit and rollback triggers + * installed by the savepoint. + */ +static void +txn_destroy_svp(struct txn_savepoint *svp) +{ + if (svp->has_triggers) { + trigger_clear(&svp->on_commit); + trigger_clear(&svp->on_rollback); + } +} + static int txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request) { @@ -144,6 +178,11 @@ txn_stmt_unref_tuples(struct txn_stmt *stmt) static void txn_rollback_to_svp(struct txn *txn, struct txn_savepoint *svp) { + /* + * Undo changes done to the database by the rolled back + * statements. Don't release the tuples as rollback triggers + * might still need them. + */ struct txn_stmt *stmt; struct stailq rollback; stailq_cut_tail(&txn->stmts, svp->stmt, &rollback); @@ -161,10 +200,31 @@ txn_rollback_to_svp(struct txn *txn, struct txn_savepoint *svp) assert(txn->n_applier_rows > 0); txn->n_applier_rows--; } - txn_stmt_unref_tuples(stmt); stmt->space = NULL; stmt->row = NULL; } + /* + * Remove commit and rollback triggers installed after + * the savepoint was set and run the rollback triggers. + */ + RLIST_HEAD(on_commit); + RLIST_HEAD(on_rollback); + if (svp->has_triggers) { + rlist_cut_before(&on_commit, &txn->on_commit, + &svp->on_commit.link); + rlist_cut_before(&on_rollback, &txn->on_rollback, + &svp->on_rollback.link); + } else if (txn->has_triggers) { + rlist_splice(&on_commit, &txn->on_commit); + rlist_splice(&on_rollback, &txn->on_rollback); + txn->has_triggers = false; + } + txn_run_triggers(txn, &on_rollback); + + /* Release the tuples. */ + stailq_foreach_entry(stmt, &rollback, next) + txn_stmt_unref_tuples(stmt); + if (txn->psql_txn != NULL) txn->psql_txn->fk_deferred_count = svp->fk_deferred_count; } @@ -362,6 +422,7 @@ txn_commit_stmt(struct txn *txn, struct request *request) goto fail; } --txn->in_sub_stmt; + txn_destroy_svp(&txn->sub_stmt_begin[txn->in_sub_stmt]); return 0; fail: txn_rollback_stmt(txn); @@ -371,7 +432,7 @@ fail: /* * A helper function to process on_commit/on_rollback triggers. */ -static inline void +static void txn_run_triggers(struct txn *txn, struct rlist *trigger) { /* Rollback triggers must not throw. */ @@ -589,8 +650,9 @@ txn_rollback_stmt(struct txn *txn) { if (txn == NULL || txn->in_sub_stmt == 0) return; - txn->in_sub_stmt--; - txn_rollback_to_svp(txn, &txn->sub_stmt_begin[txn->in_sub_stmt]); + struct txn_savepoint *svp = &txn->sub_stmt_begin[--txn->in_sub_stmt]; + txn_rollback_to_svp(txn, svp); + txn_destroy_svp(svp); } void @@ -790,9 +852,5 @@ txn_on_yield(struct trigger *trigger, void *event) struct txn *txn = in_txn(); assert(txn != NULL && !txn->can_yield); txn_rollback_to_svp(txn, &zero_svp); - if (txn->has_triggers) { - txn_run_triggers(txn, &txn->on_rollback); - txn->has_triggers = false; - } txn->is_aborted_by_yield = true; } diff --git a/src/box/txn.h b/src/box/txn.h index a1685b5f..80463ff0 100644 --- a/src/box/txn.h +++ b/src/box/txn.h @@ -89,6 +89,8 @@ struct txn_savepoint { * creation. */ int in_sub_stmt; + /** True if commit/rollback triggers are set. */ + bool has_triggers; /** * Statement, on which a savepoint is created. On rollback * to this savepoint all newer statements are rolled back. @@ -106,6 +108,15 @@ struct txn_savepoint { * state violating any number of deferred FK constraints. */ uint32_t fk_deferred_count; + /** + * Dummy commit and rollback triggers installed when this + * savepoint was created. They are used on rollback to this + * savepoint in order to remove triggers set after this + * savepoint was created. We don't set the fake triggers + * if the transaction doesn't have any triggers, because + * in that case we have to remove all triggers on rollback. + */ + struct trigger on_commit, on_rollback; }; extern double too_long_threshold; diff --git a/test/box/transaction.result b/test/box/transaction.result index 9a48f2f7..1ed649d2 100644 --- a/test/box/transaction.result +++ b/test/box/transaction.result @@ -778,3 +778,42 @@ box.begin() drop() box.rollback() box.begin() drop() box.commit() --- ... +-- +-- gh-4364: DDL doesn't care about savepoints. +-- +test_run:cmd("setopt delimiter ';'") +--- +- true +... +box.begin() +s1 = box.schema.create_space('test1') +save = box.savepoint() +s2 = box.schema.create_space('test2') +check1 = box.space.test1 ~= nil +check2 = box.space.test2 ~= nil +box.rollback_to_savepoint(save) +check3 = box.space.test1 ~= nil +check4 = box.space.test2 ~= nil +box.commit(); +--- +... +test_run:cmd("setopt delimiter ''"); +--- +- true +... +check1, check2, check3, check4 +--- +- true +- true +- true +- false +... +-- +-- gh-4365: DDL reverted by yield triggers crash. +-- +box.begin() box.execute([[CREATE TABLE test(id INTEGER PRIMARY KEY AUTOINCREMENT)]]) fiber.yield() +--- +... +box.rollback() +--- +... diff --git a/test/box/transaction.test.lua b/test/box/transaction.test.lua index 0792cc3c..a0014c9f 100644 --- a/test/box/transaction.test.lua +++ b/test/box/transaction.test.lua @@ -415,3 +415,26 @@ box.begin() create() box.rollback() box.begin() create() box.commit() box.begin() drop() box.rollback() box.begin() drop() box.commit() + +-- +-- gh-4364: DDL doesn't care about savepoints. +-- +test_run:cmd("setopt delimiter ';'") +box.begin() +s1 = box.schema.create_space('test1') +save = box.savepoint() +s2 = box.schema.create_space('test2') +check1 = box.space.test1 ~= nil +check2 = box.space.test2 ~= nil +box.rollback_to_savepoint(save) +check3 = box.space.test1 ~= nil +check4 = box.space.test2 ~= nil +box.commit(); +test_run:cmd("setopt delimiter ''"); +check1, check2, check3, check4 + +-- +-- gh-4365: DDL reverted by yield triggers crash. +-- +box.begin() box.execute([[CREATE TABLE test(id INTEGER PRIMARY KEY AUTOINCREMENT)]]) fiber.yield() +box.rollback() -- 2.11.0