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 79C31445320 for ; Fri, 17 Jul 2020 08:08:46 +0300 (MSK) References: <1594821336-14468-1-git-send-email-alyapunov@tarantool.org> <1594821336-14468-14-git-send-email-alyapunov@tarantool.org> From: Aleksandr Lyapunov Message-ID: <9236c217-eaa5-28be-6e14-953b18aa0efa@tarantool.org> Date: Fri, 17 Jul 2020 08:08:44 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US 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: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org Hi, thanks for review! On 7/17/20 1:26 AM, Vladislav Shpilevoy wrote: > Th > 1. You can pass tx_manager_use_mvcc_engine as a second > argument to txn_can_yield() instead of adding one another > 'if'. Actually it's not so simple. It would fall in assert. > >> >> + 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. See above. It's just a common restore-to-the-original state semantics. > >> >> 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. The same! > @@ -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? Good, replaced with assert. > >> + >> 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; >> } >> >>