From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 8 Jul 2019 12:57:46 +0300 From: Vladimir Davydov Subject: Re: [PATCH 2/5] txn: run on_rollback triggers on txn_abort Message-ID: <20190708095746.uay7r44jxhhk5xeh@esperanza> References: <20190708093201.GC8512@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190708093201.GC8512@atlas> To: Konstantin Osipov Cc: tarantool-patches@freelists.org List-ID: On Mon, Jul 08, 2019 at 12:32:01PM +0300, Konstantin Osipov wrote: > * Vladimir Davydov [19/07/05 23:27]: > > When a memtx transaction is aborted on yield, it isn't enough to > > rollback individual statements - we must also run on_rollback triggers, > > otherwise changes done to the schema by an aborted DDL transaction will > > be visible to other fibers until an attempt to commit it is made. > > --- > > src/box/txn.c | 7 ++++++- > > test/box/transaction.result | 24 ++++++++++++++++++++++++ > > test/box/transaction.test.lua | 10 ++++++++++ > > 3 files changed, 40 insertions(+), 1 deletion(-) > > > > diff --git a/src/box/txn.c b/src/box/txn.c > > index 818f405b..5d80833a 100644 > > --- a/src/box/txn.c > > +++ b/src/box/txn.c > > @@ -346,6 +346,7 @@ txn_run_triggers(struct txn *txn, struct rlist *trigger) > > * Some triggers require for in_txn variable to be set so > > * restore it for the time triggers are in progress. > > */ > > + struct txn *old_txn = in_txn(); > > This manipulation should be in txn_abort(), not in > txn_run_triggers(). It's txn_abort(). But this function is also called from completion callback, where it has to set the txn as well. That's why I put it there, otherwise we would have to set/restore txn context in txn_complete as well. I'm not really against it - just pointint it out. I'll prepare a patch that does that, see how it looks. > > fiber_set_txn(fiber(), txn); > > /* Rollback triggers must not throw. */ > > if (trigger_run(trigger, txn) != 0) { > > @@ -357,7 +358,7 @@ txn_run_triggers(struct txn *txn, struct rlist *trigger) > > unreachable(); > > panic("commit/rollback trigger failed"); > > } > > - fiber_set_txn(fiber(), NULL); > > + fiber_set_txn(fiber(), old_txn); > > Ideally we should never need to restore old_txn. All transaction > statements, like txn_begin() or txn_abort() should set the txn, > and whenever the transaction yields, the txn should be cleared. But we do want the transaction to remain attached to the fiber once it resumes its execution so that we can raise an error on 'commit'. Actually, we used to clear txn on yield, but then it was reworked to make 'commit' more user-friendly.