On Thursday, June 20, 2019 4:03:52 PM MSK Vladimir Davydov wrote:
> On Thu, Jun 20, 2019 at 12:23:11AM +0300, Georgy Kirichenko wrote:
> > Don't touch a fiber gc storage on a transaction rollback explicitly.
> > This relaxes dependencies between fiber and transaction life cycles.
> > 
> > Prerequisites: #1254
> > ---
> > 
> >  src/box/applier.cc     |  8 +++++---
> >  src/box/box.cc         | 14 +++++++++-----
> >  src/box/call.c         | 22 ++++++++++++++++------
> >  src/box/memtx_engine.c |  3 ++-
> >  src/box/txn.c          | 35 +++++++++++++++++------------------
> >  src/box/txn.h          |  8 ++++++--
> >  src/box/vy_scheduler.c | 10 +++++++---
> >  7 files changed, 62 insertions(+), 38 deletions(-)
> > 
> > diff --git a/src/box/txn.c b/src/box/txn.c
> > index 39b1ed773..21f7e98b4 100644
> > --- a/src/box/txn.c
> > +++ b/src/box/txn.c
> > @@ -168,6 +168,10 @@ txn_new()
> > 
> >  inline static void
> >  txn_free(struct txn *txn)
> >  {
> > 
> > +	struct txn_stmt *stmt;
> > +	stailq_foreach_entry(stmt, &txn->stmts, next)
> > +		txn_stmt_unref_tuples(stmt);
> > +
> 
> Clearly, this doesn't belong here. Should be a part of the previous
> patch?
I think it would be better if I moved this to a dedicated refactoring patch.
> 
> >  	/* Truncate region up to struct txn size. */
> >  	region_truncate(&txn->region, sizeof(struct txn));
> >  	stailq_add(&txn_cache, &txn->in_txn_cache);
> > 
> > @@ -636,6 +634,7 @@ txn_on_stop(struct trigger *trigger, void *event)
> > 
> >  {
> >  
> >  	(void) trigger;
> >  	(void) event;
> > 
> > -	txn_rollback();                 /* doesn't yield or fail */
> > +	txn_rollback(in_txn());                 /* doesn't yield or fail */
> > +
> 
> Extra new line.
> 
> >  }
> > 
> > diff --git a/src/box/txn.h b/src/box/txn.h
> > index 5a66f8e53..569978ce9 100644
> > --- a/src/box/txn.h
> > +++ b/src/box/txn.h
> > @@ -267,6 +270,7 @@ txn_on_rollback(struct txn *txn, struct trigger
> > *trigger)> 
> >  /**
> >  
> >   * Start a new statement.
> > 
> > + * Return a new statement or NULL in case of error.
> 
> Again, looks like a leftover from the prevoius patch.
> 
> Other than that the patch looks fine to me. I would rather remove
> fiber_gc() altogether, but as Kostja pointed out, it's too risky
> so fine - let it be.
Accepted
> 
> >   */
> >  
> >  struct txn_stmt *
> >  txn_begin_stmt(struct txn *txn, struct space *space);