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

[Konstantin Osipov <kostja@scylladb.com>]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/12 17:57]:

This is a very nice patch. Thank you for addressing my comments.
The bit fields is going to be trivial, so go ahead and don't
postpone it - it doesn't need a review.

One more comment below.

> diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> index e0de65d0..7635c84c 100644
> --- a/src/box/vinyl.c
> +++ b/src/box/vinyl.c
> @@ -1093,6 +1093,7 @@ static int
>  vinyl_space_check_format(struct space *space, struct tuple_format *format)
>  {
>  	struct vy_env *env = vy_env(space->engine);
> +	struct txn *txn = in_txn();
>  
>  	/*
>  	 * If this is local recovery, the space was checked before
> @@ -1121,6 +1122,8 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
>  	bool need_wal_sync;
>  	tx_manager_abort_writers_for_ddl(env->xm, space, &need_wal_sync);
>  
> +	txn_can_yield(txn, true);

I keep forgetting how this can happen, and then remember it's a
*memtx* transaction.

Please add this comment:

 /*
  * Tarantool doesn't allow multi-engine transactions, and DDL
  * system tables are memtx tables. Memtx transactions,
  * generally, can't yield.
  * So here we're in the middle of a *memtx* transaction. We don't
  * start a hidden vinyl transaction for DDL either, to avoid its
  * overhead. But some long DDL operations can yield, like
  * checking a format or building an index.
  * Unless we switch off memtx yield rollback triggers, such
  * yield leads to memtx transaction rollback. It is safe to switch
  * the trigger off though: it protects subsequent memtx
  * transactions from reading a dirty state, and at this phase
  * vinyl DDL does not change the data * dictionary, so there is
  * no dirty state that can be observed.
  */

-- 
Konstantin Osipov, Moscow, Russia