From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (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 DE3FD445320 for ; Fri, 17 Jul 2020 01:26:01 +0300 (MSK) References: <1594821336-14468-1-git-send-email-alyapunov@tarantool.org> <1594821336-14468-14-git-send-email-alyapunov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 17 Jul 2020 00:26:00 +0200 MIME-Version: 1.0 In-Reply-To: <1594821336-14468-14-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 v3 13/13] tmx: 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 5 comments below. On 15.07.2020 15:55, Aleksandr Lyapunov wrote: > Use mvcc transaction engine in memtx if the engine is enabled. > > Closes #4897 > --- > src/box/memtx_engine.c | 40 ++++++++++++++++++++++++++++++++++++---- > src/box/memtx_space.c | 28 ++++++++++++++++++++++++---- > src/box/txn.c | 3 +++ > src/box/vinyl.c | 14 ++++++++++---- > 4 files changed, 73 insertions(+), 12 deletions(-) > > diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c > index dfd6fce..1402dd7 100644 > --- a/src/box/memtx_engine.c > +++ b/src/box/memtx_engine.c > @@ -335,10 +335,39 @@ static int > memtx_engine_begin(struct engine *engine, struct txn *txn) > { > (void)engine; > - txn_can_yield(txn, false); > + if (!tx_manager_use_mvcc_engine) > + txn_can_yield(txn, false); 1. You can pass tx_manager_use_mvcc_engine as a second argument to txn_can_yield() instead of adding one another 'if'. > 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) > + 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) { > + ssize_t bsize = txm_history_commit_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,16 @@ 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; > uint32_t index_count; > > /* Only roll back the changes if they were made. */ > if (stmt->engine_savepoint == NULL) > return; > > + if (stmt->add_story != NULL || stmt->del_story != NULL) > + return txm_history_rollback_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) > @@ -914,8 +946,8 @@ static const struct engine_vtab memtx_engine_vtab = { > /* .complete_join = */ memtx_engine_complete_join, > /* .begin = */ memtx_engine_begin, > /* .begin_statement = */ generic_engine_begin_statement, > - /* .prepare = */ generic_engine_prepare, > - /* .commit = */ generic_engine_commit, > + /* .prepare = */ memtx_engine_prepare, > + /* .commit = */ memtx_engine_commit, > /* .rollback_statement = */ memtx_engine_rollback_statement, > /* .rollback = */ generic_engine_rollback, > /* .switch_to_ro = */ generic_engine_switch_to_ro, > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c > index e48ed3a..66f28c9 100644 > --- a/src/box/memtx_space.c > +++ b/src/box/memtx_space.c > @@ -260,6 +260,20 @@ memtx_space_replace_all_keys(struct space *space, struct tuple *old_tuple, > if (pk == NULL) > return -1; > assert(pk->def->opts.is_unique); > + > + if (tx_manager_use_mvcc_engine) { > + struct txn *txn = in_txn(); > + struct txn_stmt *stmt = > + txn == NULL ? NULL : txn_current_stmt(txn); > + if (stmt != NULL) { > + return txm_history_add_stmt(stmt, old_tuple, new_tuple, > + mode, result); > + } else { > + /** Ephemeral space */ > + assert(space->def->id == 0); > + } > + } > + > /* > * If old_tuple is not NULL, the index has to > * find and delete it, or return an error. > @@ -896,7 +910,9 @@ memtx_space_check_format(struct space *space, struct tuple_format *format) > if (txn_check_singlestatement(txn, "space format check") != 0) > return -1; > > - txn_can_yield(txn, true); > + bool could_yield = txn_has_flag(txn, TXN_CAN_YIELD); > + if (!could_yield) > + txn_can_yield(txn, true); 2. This change looks unnecessary. You just added 'if'. And still the result is that the txn can yield. > > struct memtx_engine *memtx = (struct memtx_engine *)space->engine; > struct memtx_ddl_state state; > @@ -940,7 +956,8 @@ memtx_space_check_format(struct space *space, struct tuple_format *format) > iterator_delete(it); > diag_destroy(&state.diag); > trigger_clear(&on_replace); > - txn_can_yield(txn, false); > + if (!could_yield) > + txn_can_yield(txn, false); 3. The same. And the same in the 2 hunks below. > return rc; > } > > @@ -1054,7 +1071,9 @@ memtx_space_build_index(struct space *src_space, struct index *new_index, > if (txn_check_singlestatement(txn, "index build") != 0) > return -1; > > - txn_can_yield(txn, true); > + bool could_yield = txn_has_flag(txn, TXN_CAN_YIELD); > + if (!could_yield) > + txn_can_yield(txn, true); > > struct memtx_engine *memtx = (struct memtx_engine *)src_space->engine; > struct memtx_ddl_state state; > @@ -1132,7 +1151,8 @@ memtx_space_build_index(struct space *src_space, struct index *new_index, > iterator_delete(it); > diag_destroy(&state.diag); > trigger_clear(&on_replace); > - txn_can_yield(txn, false); > + if (!could_yield) > + txn_can_yield(txn, false); > return rc; > } > > diff --git a/src/box/txn.c b/src/box/txn.c > index b4888b3..b9a190f 100644 > --- a/src/box/txn.c > +++ b/src/box/txn.c > @@ -203,6 +203,9 @@ txn_stmt_new(struct region *region) > static inline void > txn_stmt_destroy(struct txn_stmt *stmt) > { > + if (stmt->add_story != NULL || stmt->del_story != NULL) > + txm_history_rollback_stmt(stmt); 4. Doing rollback from a destructor is totally cursed. Why are you doing it from there? > + > if (stmt->old_tuple != NULL) > tuple_unref(stmt->old_tuple); > if (stmt->new_tuple != NULL) > diff --git a/src/box/vinyl.c b/src/box/vinyl.c > index f9252f1..f69d3d9 100644 > --- a/src/box/vinyl.c > +++ b/src/box/vinyl.c > @@ -1084,7 +1084,9 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format) > return -1; > > /* See the comment in vinyl_space_build_index(). */ > - txn_can_yield(txn, true); > + bool could_yield = txn_has_flag(txn, TXN_CAN_YIELD); > + if (!could_yield) > + txn_can_yield(txn, true); 5. Unnecessary change. The same for all the other similar places. > > struct trigger on_replace; > struct vy_check_format_ctx ctx; > @@ -1136,7 +1138,8 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format) > out: > diag_destroy(&ctx.diag); > trigger_clear(&on_replace); > - txn_can_yield(txn, false); > + if (!could_yield) > + txn_can_yield(txn, false); > return rc; > } > > @@ -4183,7 +4186,9 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index, > * change the data dictionary, so there is no dirty state > * that can be observed. > */ > - txn_can_yield(txn, true); > + bool could_yield = txn_has_flag(txn, TXN_CAN_YIELD); > + if (!could_yield) > + txn_can_yield(txn, true); > > /* > * Iterate over all tuples stored in the space and insert > @@ -4284,7 +4289,8 @@ vinyl_space_build_index(struct space *src_space, struct index *new_index, > out: > diag_destroy(&ctx.diag); > trigger_clear(&on_replace); > - txn_can_yield(txn, false); > + if (!could_yield) > + txn_can_yield(txn, false); > return rc; > } > >