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 053852B5C6 for ; Thu, 11 Apr 2019 02:43:54 -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 sHS2-B27kGGn for ; Thu, 11 Apr 2019 02:43:53 -0400 (EDT) Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 A3D682776A for ; Thu, 11 Apr 2019 02:43:53 -0400 (EDT) Date: Thu, 11 Apr 2019 09:43:51 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 2/2] Get rid of aurocommit from a txn structure Message-ID: <20190411064351.GQ8268@chai> References: <3b4ffe843914867d84b8b136ffb892bc580d8581.1554880565.git.georgy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3b4ffe843914867d84b8b136ffb892bc580d8581.1554880565.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/04/10 10:26]: > Move transaction auto start and auto commit behavior to box level. > Now transaction won't start and commit automatically without > txn_begin/txn_commit invocations. This is a part of a bigger transaction > refactoring in order to implement detachable transactions and a parallel > applier. This change is looking good, the only nit is about so much labor one has to do to run an autocommit transaction. If it was C++, I would suggest adding a lambda which would auto-commit on destruction, But given it's plain C, how about simply making the code a bit more straightforward in its flow ? > + if (txn_commit_stmt(txn, request) != 0 || > + (autocommit && txn_commit(txn) != 0)) > + goto fail; > + if (autocommit) > + fiber_gc(); > + This piece in particular, I would rewrite: if (txn_commit_stmt()) goto fail() if (autocommit) { if (txn_commit() goto fail; fiber_gc(); } > + rc = 0; > +done: > + if (tuple != NULL) > + tuple_unref(tuple); > return rc; > + > +fail: > + if (autocommit) > + txn_rollback(); > + goto done; I think using double labels like here is on the verge of goto abuse. > - if (box_process_rw(&request, space, NULL) != 0) { > + struct txn *txn = txn_begin(); > + if (txn == NULL || box_process_rw(&request, space, NULL) != 0 || > + txn_commit(txn) != 0) { > say_error("error applying row: %s", request_str(&request)); > + if (txn != NULL) > + txn_rollback(); > diag_raise(); Here is another example. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov