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 E228F3025E for ; Fri, 31 May 2019 15:21:49 -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 NwkDZN0V8W8e for ; Fri, 31 May 2019 15:21:49 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 3C7C930243 for ; Fri, 31 May 2019 15:21:49 -0400 (EDT) Date: Fri, 31 May 2019 22:21:37 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v2 2/8] Get rid of autocommit from a txn structure Message-ID: <20190531192137.GA6141@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]: apply_initial_join_row(struct xrow_header *row) > { > + struct txn *txn = txn_begin(); > + if (txn == NULL) > + return -1; > struct request request; > xrow_decode_dml(row, &request, dml_request_key_map(row->type)); > - struct space *space = space_cache_find_xc(request.space_id); > + struct space *space = space_cache_find(request.space_id); > + if (space == NULL) > + return -1; Missing txn_rollback(). > /* no access checks here - applier always works with admin privs */ > - return space_apply_initial_join_row(space, &request); > + if (space_apply_initial_join_row(space, &request)) { > + txn_rollback(); > + return -1; > + } > + int rc = txn_commit(txn); > + fiber_gc(); > + return rc; > } > > /** > @@ -189,8 +200,8 @@ static int > box_process_rw(struct request *request, struct space *space, > struct tuple **result) > { > + struct tuple *tuple = NULL; > + struct txn *txn = in_txn(); > + bool is_autocommit = txn == NULL; > + if (is_autocommit && (txn = txn_begin()) == NULL) > + 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(txn, space) == NULL) > + goto fail; > if (space_execute_dml(space, txn, request, &tuple) != 0) { > - txn_rollback_stmt(); > - return -1; > + txn_rollback_stmt(txn); > + 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. > */ Please move the comment to its place, where we actually pin the tuple. > + 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(txn) != 0) > + return -1; > + fiber_gc(); > + return 0; > + } This code flow can be a bit more straightforward, I sent a diff to the list in a separate email. > tuple_ref(tuple); > - int rc = txn_commit_stmt(txn, request); > - if (rc == 0) > - tuple_bless(tuple); > + > + if (txn_commit_stmt(txn, request)) { > + /* Unref tuple and rollback if autocommit. */ > + tuple_unref(tuple); > + goto fail; > + } > + if (is_autocommit) { > + if (txn_commit(txn) != 0) { > + /* Unref tuple and exit. */ > + tuple_unref(tuple); > + return -1; > + } > + fiber_gc(); > + } > + tuple_bless(tuple); > tuple_unref(tuple); > - return rc; > + return 0; > + > +fail: > + if (is_autocommit) > + txn_rollback(); > + return -1; > } > > void > @@ -299,8 +330,12 @@ apply_wal_row(struct xstream *stream, struct xrow_header *row) > xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type)); > if (request.type != IPROTO_NOP) { > struct space *space = space_cache_find_xc(request.space_id); > - 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(); Missing fiber_gc() after txn_commit() > diag_raise(); > } > } > txn_commit_ro_stmt(txn); > @@ -1313,8 +1348,15 @@ box_sequence_reset(uint32_t seq_id) > static inline void > box_register_replica(uint32_t id, const struct tt_uuid *uuid) > { > + struct txn *txn = txn_begin(); > + if (txn == NULL) > + diag_raise(); > if (boxk(IPROTO_INSERT, BOX_CLUSTER_ID, "[%u%s]", > - (unsigned) id, tt_uuid_str(uuid)) != 0) > + (unsigned) id, tt_uuid_str(uuid)) != 0) { > + txn_rollback(); > + diag_raise(); > + } > + if (txn_commit(txn) != 0) > diag_raise(); Missing fiber_gc() after txn_commit(). > assert(replica_by_uuid(uuid)->id == id); > } > @@ -1636,9 +1678,16 @@ box_set_replicaset_uuid(const struct tt_uuid *replicaset_uuid) > uu = *replicaset_uuid; > else > tt_uuid_create(&uu); > + struct txn *txn = txn_begin(); > + if (txn == NULL) > + diag_raise(); > /* Save replica set UUID in _schema */ > if (boxk(IPROTO_INSERT, BOX_SCHEMA_ID, "[%s%s]", "cluster", > - tt_uuid_str(&uu))) > + tt_uuid_str(&uu))) { > + txn_rollback(); > + diag_raise(); > + } > + if (txn_commit(txn) != 0) > diag_raise(); Missing fiber_gc(). > --- a/src/box/journal.h > +++ b/src/box/journal.h > @@ -32,6 +32,7 @@ > */ > #include > #include > +#include "trigger.h" > #include "salad/stailq.h" Stray change, please remove. > > - if (!txn->is_autocommit) { > - trigger_create(&txn->fiber_on_stop, txn_on_stop, NULL, NULL); > - trigger_add(&fiber()->on_stop, &txn->fiber_on_stop); > - } > + trigger_create(&txn->fiber_on_stop, txn_on_stop, NULL, NULL); > + trigger_add(&fiber()->on_stop, &txn->fiber_on_stop); Come to think of it, I see no reason why txn->is_autocommit flag is removed. We can pass is_autocommit property to txn, even though it doesn't behave in a special way on autocommit. Or we could rename the option to set_on_yield_on_stop. Now the triggers are set and cleared even for trivial statements using memtx. This could be a performance issue. Did you benchmark your patch, it may introduce a yet another regression. Please ask @avitikhon to help with benchmarks, if there is a slow down from setting the triggers all the time, we can fix the patch to not do it, otherwise it's ok to simply set the triggers always. > --- a/src/box/wal.c > +++ b/src/box/wal.c > @@ -303,7 +303,6 @@ tx_schedule_rollback(struct cmsg *msg) > * in-memory database state. > */ > stailq_reverse(&writer->rollback); > - /* Must not yield. */ > tx_schedule_queue(&writer->rollback); > stailq_create(&writer->rollback); > if (msg != &writer->in_rollback) Please explain in the changeset comment why you removed this comment. Or is this a stray change? -- Konstantin Osipov, Moscow, Russia