From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id AF7F4305D9 for ; Thu, 20 Jun 2019 03:43:20 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id fwK7kMKnvxOG for ; Thu, 20 Jun 2019 03:43:20 -0400 (EDT) Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 70851305B3 for ; Thu, 20 Jun 2019 03:43:20 -0400 (EDT) Date: Thu, 20 Jun 2019 10:43:18 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v4 4/9] txn: get rid of fiber_gc from txn_rollback Message-ID: <20190620074318.GD15155@atlas> References: <0eeb7c2b0e45bfae1cba4488292c7d6fc7e69fa4.1560978655.git.georgy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0eeb7c2b0e45bfae1cba4488292c7d6fc7e69fa4.1560978655.git.georgy@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Georgy Kirichenko * Georgy Kirichenko [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