Re: [Tarantool-patches] [PATCH 3/7] txn: detach transaction from fiber.

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Thu, Aug 05, 2021 at 09:17:41PM +0300, mechanik20051988 wrote:
> From: mechanik20051988 <mechanik20.05.1988@gmail.com>
> 
> Detach transaction from fiber: now it is possible to start
> transaction in fiber, save the pointer to it in some place
> independent from the fiber and then restore it other fiber.

> For this purpose `fiber_on_stop` and `fiber_on_yield` triggers
> was changed: previously they received pointer to transaction
> from the fiber storage, now they receive it as their argument,
> because fiber storage can be already empty when they called.

This is not true anymore, because you clear both triggers on detach.
Please revert this part.

> Also implement function `txn_detach`, which clear `fiber_on_stop`
> and `fiber_on_yeild` triggers. Detached transaction does not
> rollback in case when fiber stopped, but can be aborted in case
> it does not support yeild.
> 
> Path of #5860
> ---
>  src/box/txn.c | 29 ++++++++++++++++++++++-------
>  src/box/txn.h | 11 +++++++++++
>  2 files changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/txn.c b/src/box/txn.c
> index b80e722a4..de6a7f37d 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -302,8 +302,9 @@ txn_begin(void)
>  	rlist_create(&txn->savepoints);
>  	txn->fiber = NULL;
>  	fiber_set_txn(fiber(), txn);
> -	/* fiber_on_yield is initialized by engine on demand */
> -	trigger_create(&txn->fiber_on_stop, txn_on_stop, NULL, NULL);
> +	trigger_create(&txn->fiber_on_yield, txn_on_yield, txn, NULL);
> +	trigger_create(&txn->fiber_on_stop, txn_on_stop, txn, NULL);
> +	/* fiber_on_yield is added by engine on demand */
>  	trigger_add(&fiber()->on_stop, &txn->fiber_on_stop);
>  	/*
>  	 * By default all transactions may yield.
> @@ -1016,7 +1017,6 @@ txn_can_yield(struct txn *txn, bool set)
>  		trigger_clear(&txn->fiber_on_yield);
>  	} else if (!set && could) {
>  		txn_clear_flags(txn, TXN_CAN_YIELD);
> -		trigger_create(&txn->fiber_on_yield, txn_on_yield, NULL, NULL);
>  		trigger_add(&fiber()->on_yield, &txn->fiber_on_yield);
>  	}
>  	return could;
> @@ -1216,9 +1216,8 @@ txn_savepoint_release(struct txn_savepoint *svp)
>  static int
>  txn_on_stop(struct trigger *trigger, void *event)
>  {
> -	(void) trigger;
>  	(void) event;
> -	struct txn *txn = in_txn();
> +	struct txn *txn = trigger->data;
>  	assert(txn->signature == TXN_SIGNATURE_UNKNOWN);
>  	txn->signature = TXN_SIGNATURE_ROLLBACK;
>  	txn_rollback(txn);
> @@ -1246,12 +1245,28 @@ txn_on_stop(struct trigger *trigger, void *event)
>  static int
>  txn_on_yield(struct trigger *trigger, void *event)
>  {
> -	(void) trigger;
>  	(void) event;
> -	struct txn *txn = in_txn();
> +	struct txn *txn = trigger->data;
>  	assert(txn != NULL);
>  	assert(!txn_has_flag(txn, TXN_CAN_YIELD));
>  	txn_rollback_to_svp(txn, NULL);
>  	txn_set_flags(txn, TXN_IS_ABORTED_BY_YIELD);
>  	return 0;
>  }
> +
> +struct txn *
> +txn_detach(void)
> +{
> +	struct txn *txn = in_txn();
> +	if (txn == NULL)
> +		return NULL;
> +	if (!txn_has_flag(txn, TXN_CAN_YIELD)) {
> +		struct trigger trigger;
> +		trigger.data = txn;
> +		txn_on_yield(&trigger, NULL);
> +	}
> +	trigger_clear(&txn->fiber_on_yield);

This trigger needs to be cleared only if TXN_CAN_YIELD is unset.
Then you don't need to modify txn_begin and txn_can_yield.

> +	trigger_clear(&txn->fiber_on_stop);
> +	fiber_set_txn(fiber(), NULL);
> +	return txn;
> +}
> \ No newline at end of file

Please fix.

> diff --git a/src/box/txn.h b/src/box/txn.h
> index 8741dc6a1..b8bba67b8 100644
> --- a/src/box/txn.h
> +++ b/src/box/txn.h
> @@ -457,6 +457,17 @@ fiber_set_txn(struct fiber *fiber, struct txn *txn)
>  	fiber->storage.txn = txn;
>  }
>  
> +/**
> + * Detach transaction from fiber.
> + * By default if the fiber is stopped the transaction
> + * started in this fiber is rollback. This function
> + * detaches transaction from fiber, leaving it alive
> + * after fiber is stopped.
> + * @pre txn == in_txn()
> + */
> +struct txn *
> +txn_detach(void);
> +

I think we should introduce txn_attach, for symmetry and so as not
access fiber storage directly from the iproto code, but to be sure I
need to see the patch that makes use of this function, which is the next
to the last one in the series...

This means that there's no point in this patch alone - it doesn't ease
the review process, because to figure out how the function it introduces
is going to be used, the reviewer needs to look at a following patch.
Besides, the code it introduces is never used nor tested until the rest
of the series is applied so it can't be committed right now.

That being said, please fold this patch into the commit introducing
transactions in the protocol.