From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 10 Jul 2019 23:34:39 +0300 From: Konstantin Osipov Subject: Re: [PATCH v2 1/3] memtx: fix txn_on_yield for DDL transactions Message-ID: <20190710203439.GN5619@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: * Vladimir Davydov [19/07/10 16:13]: > static int > memtx_engine_begin(struct engine *engine, struct txn *txn) > { > (void)engine; > + txn_disable_yield(txn); This name is rather obscure. txn_prohibit_yield() txn_rollback_on_yield() txn_disallow_yield() txn_set_yield_trigger() txn_set_rollback_on_yield_trigger() txn_can_yield(false); > - /* .prepare = */ memtx_engine_prepare, > + /* .begin_statement = */ generic_engine_begin_statement, > + /* .prepare = */ generic_engine_prepare, If some of these callbacks are now empty in all engines, they should be removed from vtab. > + txn_enable_yield_for_ddl(); > + Same here: txn_allow_yield() txn_yield_exception(); txn_can_yield(true); > struct memtx_engine *memtx = (struct memtx_engine *)space->engine; > struct memtx_ddl_state state; > state.format = format; > @@ -930,6 +932,7 @@ memtx_space_check_format(struct space *space, struct tuple_format *format) > iterator_delete(it); > diag_destroy(&state.diag); > trigger_clear(&on_replace); > + txn_disable_yield_after_ddl(); txn_can_yield(false); > + > static inline void > fiber_set_txn(struct fiber *fiber, struct txn *txn) > { > @@ -194,6 +200,7 @@ txn_begin() > txn->has_triggers = false; > txn->is_done = false; > txn->is_aborted = false; > + txn->is_yield_disabled = false; It is hight time to convert these to bit fields. txn->can_yield > txn->in_sub_stmt = 0; > txn->id = ++tsn; > txn->signature = -1; > @@ -201,8 +208,8 @@ txn_begin() > txn->engine_tx = NULL; > txn->psql_txn = NULL; > txn->fiber = NULL; > - /* fiber_on_yield/fiber_on_stop initialized by engine on demand */ > fiber_set_txn(fiber(), txn); > + /* fiber_on_yield is initialized by engine on demand */ > trigger_create(&txn->fiber_on_stop, txn_on_stop, NULL, NULL); > trigger_add(&fiber()->on_stop, &txn->fiber_on_stop); > return txn; > @@ -463,6 +470,12 @@ txn_write_to_wal(struct txn *txn) > static int > txn_prepare(struct txn *txn) > { > + if (txn->is_aborted) { > + assert(txn->is_yield_disabled); To anyone who doesn't know that is_aborted is only set by the yield trigger, this is rather obscure. How are these variables connected? is_aborted is a very general name, perhaps the context has changed a bit but now when I read this code I don't immediately understadn why is_aborted == ER_TRANACTION_YIELD. Perhaps is_aborted should be is_aborted_by_yield. Overall I agree the approach is much better than before, at least easier to track since the logic is not hidden in the engine. Please let's make the names more evident now, and the api simple. I think txn_can_yield(true/false) is good, and I also think that we need to collapse all txn flags into a bit field now that we have a critical mass. -- Konstantin Osipov, Moscow, Russia