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 7EF052AEAD for ; Wed, 24 Apr 2019 15:07:22 -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 sabO_8kp60gM for ; Wed, 24 Apr 2019 15:07:22 -0400 (EDT) Received: from mail-lj1-f178.google.com (mail-lj1-f178.google.com [209.85.208.178]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 110B92A474 for ; Wed, 24 Apr 2019 15:07:21 -0400 (EDT) Received: by mail-lj1-f178.google.com with SMTP id v13so5379363ljk.4 for ; Wed, 24 Apr 2019 12:07:21 -0700 (PDT) Date: Wed, 24 Apr 2019 22:07:18 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 04/10] Get rid of autocommit from a txn structure Message-ID: <20190424190718.GD21344@atlas> References: <05824da8674e6330f2c54833277d139846c235d0.1555677159.git.georgy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <05824da8674e6330f2c54833277d139846c235d0.1555677159.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/19 19:59]: > box_process_rw(struct request *request, struct space *space, > struct tuple **result) > { > + struct tuple *tuple = NULL; > + struct txn *txn = in_txn(); > + bool autocommit = txn == NULL; > + if (txn == NULL && (txn = txn_begin()) == NULL) nit: why not use if (autocommit && (txn == txn_begin()) == NULL) instead > + return -1; > assert(iproto_type_is_dml(request->type)); > rmean_collect(rmean_box, request->type, 1); > if (access_check_space(space, PRIV_W) != 0) > - return -1; > - struct txn *txn = txn_begin_stmt(space); > - if (txn == NULL) > - return -1; > - struct tuple *tuple; > + goto fail; > + if (txn_begin_stmt(space) == NULL) > + goto fail; I guess txn_begin_stmt can as well receive txn explicitly now, since we always fetch fiber->txn before calling txn_begin_stmt(). This will save us one fiber key fetch. > if (space_execute_dml(space, txn, request, &tuple) != 0) { > txn_rollback_stmt(); > - return -1; > + goto fail; > } > - if (result == NULL) > - return txn_commit_stmt(txn, request); > - *result = tuple; > - if (tuple == NULL) > - return txn_commit_stmt(txn, request); > /* > * Pin the tuple locally before the commit, > * otherwise it may go away during yield in > * when WAL is written in autocommit mode. > */ > - tuple_ref(tuple); > - int rc = txn_commit_stmt(txn, request); > - if (rc == 0) > + if (tuple != NULL) > + tuple_ref(tuple); > + > + if (result != NULL) > + *result = tuple; > + > + if (txn_commit_stmt(txn, request) != 0) > + goto fail; > + if (autocommit) { > + if (txn_commit(txn) != 0) > + goto fail; > + fiber_gc(); > + } > + > + if (tuple != NULL && result != NULL) { > tuple_bless(tuple); > - tuple_unref(tuple); The code here now looks quite messy. I think it would be easier to read if we split it between if (autocommit) ... else ... branches: if (tuple == NULL) { return is_autocommit ? txn_commit_stmt() : txn_commit(); } tuple_ref(tuple); if (txn_commit_stmt(txn, request)) goto fail; if (is_autocommit) if (txn_commit()) goto fail; fiber_gc(); } tuple_bless(tuple); if (result) *result = tuple; tuple_unref(tuple); return 0; The patch is generally looking good, please fix the above nits and it will be good to go. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov