From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id DBE00445320 for ; Wed, 15 Jul 2020 02:45:04 +0300 (MSK) References: <1594221263-6228-1-git-send-email-alyapunov@tarantool.org> <1594221263-6228-17-git-send-email-alyapunov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <11582023-5848-99ed-a5e8-b0d0eb6a9c3f@tarantool.org> Date: Wed, 15 Jul 2020 01:45:02 +0200 MIME-Version: 1.0 In-Reply-To: <1594221263-6228-17-git-send-email-alyapunov@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 16/16] tx: use new tx manager in memtx List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aleksandr Lyapunov , tarantool-patches@dev.tarantool.org Thanks for the patch! See 6 comments below. On 08.07.2020 17:14, Aleksandr Lyapunov wrote: > --- > src/box/memtx_engine.c | 49 ++++++++++++++++++++++++++++++++++++++++++------- > src/box/memtx_space.c | 12 ++++++++++++ > src/box/txn.c | 3 +++ > 3 files changed, 57 insertions(+), 7 deletions(-) > > diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c > index dfd6fce..b5c5f03 100644 > --- a/src/box/memtx_engine.c > +++ b/src/box/memtx_engine.c > @@ -339,6 +339,35 @@ memtx_engine_begin(struct engine *engine, struct txn *txn) > return 0; > } > > +static int > +memtx_engine_prepare(struct engine *engine, struct txn *txn) > +{ > + (void)engine; > + struct txn_stmt *stmt; > + stailq_foreach_entry(stmt, &txn->stmts, next) { > + if (stmt->add_story != NULL || stmt->del_story != NULL) 1. Seems like it is worth moving this check into a static inline function in the txn.h header. You use it a lot. > + txm_history_prepare_stmt(stmt); > + } > + return 0; > +} > + > +static void > +memtx_engine_commit(struct engine *engine, struct txn *txn) > +{ > + (void)engine; > + struct txn_stmt *stmt; > + stailq_foreach_entry(stmt, &txn->stmts, next) { > + if (stmt->add_story != NULL || stmt->del_story != NULL) > + { 2. Opening brace should be on the same line as 'if'. The same please find and fix in all the other places. > + ssize_t bsize = txm_history_release_stmt(stmt); > + assert(stmt->space->engine == engine); > + struct memtx_space *mspace = > + (struct memtx_space *)stmt->space; > + mspace->bsize += bsize; > + } > + } > +} > + > static void > memtx_engine_rollback_statement(struct engine *engine, struct txn *txn, > struct txn_stmt *stmt) > @@ -348,13 +377,17 @@ memtx_engine_rollback_statement(struct engine *engine, struct txn *txn, > if (stmt->old_tuple == NULL && stmt->new_tuple == NULL) > return; > struct space *space = stmt->space; > - struct memtx_space *memtx_space = (struct memtx_space *)space; > + struct memtx_space* memtx_space = (struct memtx_space*) space; 3. Unnecessary diff. > uint32_t index_count; > > /* Only roll back the changes if they were made. */ > if (stmt->engine_savepoint == NULL) > return; > > + if (memtx_space->replace == memtx_space_replace_all_keys && 4. Why is it done only in case of memtx_space_replace_all_keys? > + (stmt->add_story != NULL || stmt->del_story != NULL)) > + return txm_history_unlink_stmt(stmt); > + > if (memtx_space->replace == memtx_space_replace_all_keys) > index_count = space->index_count; > else if (memtx_space->replace == memtx_space_replace_primary_key) > @@ -362,12 +395,14 @@ memtx_engine_rollback_statement(struct engine *engine, struct txn *txn, > else > panic("transaction rolled back during snapshot recovery"); > > - for (uint32_t i = 0; i < index_count; i++) { > - struct tuple *unused; > - struct index *index = space->index[i]; > + for (uint32_t i = 0; i < index_count; i++) > + { > + struct tuple* unused; > + struct index* index = space->index[i]; 5. Unnecessary changes. Also not correct. We use 'type *' instead of 'type*'. We put { on the same line as cycle, not on a separate line. The same below and in all the other places. > /* Rollback must not fail. */ > if (index_replace(index, stmt->new_tuple, stmt->old_tuple, > - DUP_INSERT, &unused) != 0) { > + DUP_INSERT, &unused) != 0) > + { > diag_log(); > unreachable(); > panic("failed to rollback change"); > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c > index 5820c40..28552a9 100644 > --- a/src/box/memtx_space.c > +++ b/src/box/memtx_space.c > @@ -260,6 +260,18 @@ memtx_space_replace_all_keys(struct space *space, struct tuple *old_tuple, > if (pk == NULL) > return -1; > assert(pk->def->opts.is_unique); > + > + struct txn *txn = in_txn(); > + struct txn_stmt *stmt = txn == NULL ? NULL : txn_current_stmt(txn); > + if (stmt != NULL) { > + return txm_history_link_stmt(txn_current_stmt(txn), 6. Why do you get txn_current_stmt(stmt) again? You already have it in stmt variable. > + old_tuple, new_tuple, mode, > + result); > + } else { > + /** Ephemeral space */ > + assert(space->def->id == 0); > + }