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

[Konstantin Osipov <kostja@tarantool.org>]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/08 13:02]:
> > 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.

Yes, so, basically, txn_begin(), txn_abort(), txn_complete()
should manage the fiber key.

> 
> > >  	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.

I don't get it. When a transaction is resumed after a yield, it
has to set its key again anyway. Basically, transaction == fiber
is no longer true, and each time a transaction starts running it
should update its key. Better yet, let's kill the transaction key
in the fiber altogether and pass the txn around by value.

-- 
Konstantin Osipov, Moscow, Russia