From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 198071FCCC for ; Mon, 29 Jul 2019 09:11:53 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ulm8a8C421GP for ; Mon, 29 Jul 2019 09:11:52 -0400 (EDT) Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 98E5921559 for ; Mon, 29 Jul 2019 09:11:52 -0400 (EDT) Received: by smtp3.mail.ru with esmtpa (envelope-from ) id 1hs5RO-0002eR-Pu for tarantool-patches@freelists.org; Mon, 29 Jul 2019 16:11:51 +0300 Date: Mon, 29 Jul 2019 16:11:50 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint Message-ID: <20190729131150.GA16930@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org * Vladimir Davydov [19/07/19 21:09]: I spent some time thinking about this patch. I agree we need to make commit and rollback triggers, as well as some SQL related members, part of the savepoint state. DDL now can be part of a transaction, and since we may roll back to a savepoint, we need to make sure that all parts of the transaction, including a commit/rollback trigger, are part of the savepoint. However the patch itself suffers from quite a bit of rot. Initially, the idea of struct txn was very simple: it is a linked list of txn_stmt objects, and each txn_stmt object represents a change in transaction state. The history is easy to navigate over, and a savepoint is just a link inside the history chain. commit/rollback triggers were not part of this story because they were only used for DDL which was non-transactional. This code was allowed to rot and when some SQL related members were added directly to struct txn without considering the issue of sub-statements and savepoints. This patch, in this vein, instead of designing the transaction system to make sure that every part of transaction state is part of a change history, adds a few extra savepoint members and makes sure that transaction state is carefully copied to the savepoint when it is created. This is contrary to how savepoints used to work, when a savepoint was just a link in the history of changes of the transaction state. As an ugly artefact, we store such members as has_triggers, on_commit, on_rollback in struct txn twice: once in struct txn itself, and another time in txn->in_sub_stmt[0], which is initialized along with the txn. So I think that some work needs to be done to move transaction state which can participate in a savepoint to either an existing object representing a piece of the transaction history (struct txn_stmt) or a new object (txn_stmt_chain, txn_savepoint_state or something similar), the transaction is represented as a chain of such objects, the first object in the chain is a member of struct txn and is created when the transaction starts, and the savepoint can continue to be just a link in that chain. Let's discuss this when you have a chance. > 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 > -- Konstantin Osipov, Moscow, Russia