Re: [PATCH 2/5] txn: run on_rollback triggers on txn_abort

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Mon, Jul 08, 2019 at 12:32:01PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [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.