From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: =?utf-8?B?0JPQtdC+0YDQs9C40Lkg0JrQuNGA0LjRh9C10L3QutC+?= Subject: Re: [tarantool-patches] [PATCH v4 4/9] txn: get rid of fiber_gc from txn_rollback Date: Thu, 20 Jun 2019 23:16:12 +0300 Message-ID: <1861291.OFXHxrNTa2@home.lan> In-Reply-To: <20190620130352.q763jjpvkusujtq4@esperanza> References: <0eeb7c2b0e45bfae1cba4488292c7d6fc7e69fa4.1560978655.git.georgy@tarantool.org> <20190620130352.q763jjpvkusujtq4@esperanza> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart4900909.8oFiCylCWd"; micalg="pgp-sha256"; protocol="application/pgp-signature" To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: --nextPart4900909.8oFiCylCWd Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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); --nextPart4900909.8oFiCylCWd Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEFB+nbqWGnp59Rk9ZFSyY70x8X3sFAl0L6YwACgkQFSyY70x8 X3ujUQf+Ok1VX5+F7SjzROas7v5whXBdKgD5dTC6bLSkgF6YRasGBxNup+EvviaN O9pjD0QQ+bPvCPEGLIXdjavV+KWA+S4GYRpd3v1XpoZkk5/qNSWXr8VUV6MRSQeo IrEo/sbp15XzXFbOihDh9tkL3ywnHhQ4eoQR+K7gPNoNLO+/cKDS30lWXkOkRd/K IiKKB2hClR4mU7in1wrimW2F7b4m/OiuSptqLAe5uJcZYOhjTEkrYdMBY2X6HmNR 4H+H4wmgJcKGS9FX2vR0L3dlvY8ChCMUzKOGH+K5noJSrMATqAG+x/An2OF3fhvf 4H1n2hERbObNBrsGWdZL0c06xdouyA== =G6j1 -----END PGP SIGNATURE----- --nextPart4900909.8oFiCylCWd--