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/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().
>  	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.


-- 
Konstantin Osipov, Moscow, Russia