[tarantool-patches] Re: [PATCH v4 4/9] txn: get rid of fiber_gc from txn_rollback

[Konstantin Osipov <kostja@tarantool.org>]

* Georgy Kirichenko <georgy@tarantool.org> [19/06/20 09:54]:
> Don't touch a fiber gc storage on a transaction rollback explicitly.
> This relaxes dependencies between fiber and transaction life cycles.

As discussed verbally, LGTM.

> --- a/src/box/applier.cc
> +++ b/src/box/applier.cc
> @@ -190,7 +190,7 @@ apply_initial_join_row(struct xrow_header *row)
>  	fiber_gc();
>  	return rc;
>  rollback:
> -	txn_rollback();
> +	txn_rollback(txn);
>  	return -1;
>  }
>  

I weighed your argument that an error leads to diag_raise(), which
ends the running fiber, so fiber_gc() is unnecessary. This is
true, but don't you think it is a change in the protocol between
the caller and the callee? What should be this protocol generally
now that we moved most of memory allocation into txn? What should
is fiber gc useful for, and should the user always use it with
savepoints perhaps?

While we're thinking about it and inspecting all of the code to
use txn gc if at all possible, please add a comment that
fiber_gc() will be called by the caller since we expect the error
to abort it. Actually whichever you prefer -a comment or a
fiber_gc(). I prefer fiber_gc(), until we replace it with an
assert that fiber gc is not used in this function.

> @@ -334,7 +336,7 @@ apply_wal_row(struct xstream *stream, struct xrow_header *row)
>  		}
>  		if (box_process_rw(&request, space, NULL) != 0) {
>  			say_error("error applying row: %s", request_str(&request));
> -			txn_rollback();
> +			txn_rollback(txn);
>  			diag_raise();

the same argument is here. please either add a comment or not
change the protocol between the caller and the callee and leave
fiber_gc() in place. Tomorrow the error may be suppressed for
whatever reason, and we may start having growing garbage on fiber
gc pool. Honestly, until we add fiber_gc() to every fiber yield or
use another system-wide mechanism to ensure it never grows out of
control, I don't think it's safe to remove it from such places.

Fusing fiber memory and txn memory had one advantage: we had at
leasat some certainty that a fiber which executes transaction will
free its memory on a regular basis. Now there is no certainty at
all.

With these two minor comments the patch is LGTM.

-- 
Konstantin Osipov, Moscow, Russia