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

["Георгий Кириченко" <georgy@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 2482 bytes --]

On Thursday, June 20, 2019 10:43:18 AM MSK Konstantin Osipov wrote:

Accepted, I will restore all fiber_gc
> * 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.


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 488 bytes --]