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 00DAB2E121 for ; Mon, 27 May 2019 16:51:39 -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 d5EEvG8jsxxs for ; Mon, 27 May 2019 16:51:38 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 B56442E025 for ; Mon, 27 May 2019 16:51:38 -0400 (EDT) Date: Mon, 27 May 2019 23:51:34 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v2 2/8] Get rid of autocommit from a txn structure Message-ID: <20190527205134.GA15844@atlas> References: <5ac79c845931bd0bef74d3a988762ceec54ab6b4.1558598679.git.georgy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5ac79c845931bd0bef74d3a988762ceec54ab6b4.1558598679.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/05/23 12:14]: > Move transaction auto start and auto commit behavior to the box level. > >From now a 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. I started looking at this patch and I don't fully understand it. - why do you sometimes call fiber_gc() after txn_commit() and sometimes do not? It used to be called before, and not called in some cases after your change. Here's the diff (it doesn't pass tests, which baffles me, could you please explain? --- src/box/applier.cc | 4 +++- src/box/box.cc | 52 ++++++++++++++++++++++++++-------------------- src/box/journal.h | 1 - 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/src/box/applier.cc b/src/box/applier.cc index 9d2989094..75698fea7 100644 --- a/src/box/applier.cc +++ b/src/box/applier.cc @@ -178,8 +178,10 @@ apply_initial_join_row(struct xrow_header *row) struct request request; xrow_decode_dml(row, &request, dml_request_key_map(row->type)); struct space *space = space_cache_find(request.space_id); - if (space == NULL) + if (space == NULL) { + txn_rollback(); return -1; + } /* no access checks here - applier always works with admin privs */ if (space_apply_initial_join_row(space, &request)) { txn_rollback(); diff --git a/src/box/box.cc b/src/box/box.cc index 2a738fa36..a371af813 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -184,30 +184,27 @@ box_process_rw(struct request *request, struct space *space, txn_rollback_stmt(txn); goto fail; } - /* - * Pin the tuple locally before the commit, - * otherwise it may go away during yield in - * when WAL is written in autocommit mode. - */ if (result != NULL) *result = tuple; if (result == NULL || tuple == NULL) { - if (!is_autocommit) - return txn_commit_stmt(txn, request); - /* Autocommit mode. */ - if (txn_commit_stmt(txn, request) != 0) { - txn_rollback(); - return -1; + if (txn_commit_stmt(txn, request) != 0) + goto fail; + if (is_autocommit) { + if (txn_commit(txn) != 0) + return -1; + fiber_gc(); } - if (txn_commit(txn) != 0) - return -1; - fiber_gc(); return 0; } + /* + * 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); - if (txn_commit_stmt(txn, request)) { + if (txn_commit_stmt(txn, request) != 0) { /* Unref tuple and rollback if autocommit. */ tuple_unref(tuple); goto fail; @@ -328,25 +325,32 @@ apply_wal_row(struct xstream *stream, struct xrow_header *row) { struct request request; xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type)); + struct wal_stream *xstream = + container_of(stream, struct wal_stream, base); + if (request.type != IPROTO_NOP) { struct space *space = space_cache_find_xc(request.space_id); 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(); + if (txn == NULL) + goto error; + if (box_process_rw(&request, space, NULL) != 0) { + txn_rollback(); + goto error; } + if (txn_commit(txn) != 0) + goto error; + fiber_gc(); } - struct wal_stream *xstream = - container_of(stream, struct wal_stream, base); /** * Yield once in a while, but not too often, * mostly to allow signal handling to take place. */ if (++xstream->rows % xstream->yield == 0) fiber_sleep(0); +error: + say_error("error applying row: %s", request_str(&request)); + diag_raise(); + } static void @@ -1358,6 +1362,7 @@ box_register_replica(uint32_t id, const struct tt_uuid *uuid) } if (txn_commit(txn) != 0) diag_raise(); + fiber_gc(); assert(replica_by_uuid(uuid)->id == id); } @@ -1689,6 +1694,7 @@ box_set_replicaset_uuid(const struct tt_uuid *replicaset_uuid) } if (txn_commit(txn) != 0) diag_raise(); + fiber_gc(); } void diff --git a/src/box/journal.h b/src/box/journal.h index 77d3073c8..8ac32ee5e 100644 --- a/src/box/journal.h +++ b/src/box/journal.h @@ -32,7 +32,6 @@ */ #include #include -#include "trigger.h" #include "salad/stailq.h" #if defined(__cplusplus) -- Konstantin Osipov, Moscow, Russia