Re: [PATCH v2 1/3] memtx: fix txn_on_yield for DDL transactions

[Konstantin Osipov <kostja@tarantool.org>]

* Vladimir Davydov <vdavydov.dev@gmail.com> [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