Re: [PATCH 1/5] vinyl: don't sync WAL on space alter if not necessary

[Konstantin Osipov <kostja@tarantool.org>]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> Changes done to an altered space while a new index is being built or
> the format is being checked are propagated via an on_replace trigger.
> The problem is there may be transactions that started before the alter
> request. Their working set can't be checked so we simply abort them.
> We can't abort transactions that have reached WAL so we also call
> wal_sync() to flush all pending WAL requests. This is a yielding
> operation and we call it even if there's no transactions that need
> to be flushed. As a result, vinyl space alter yields unconditionally,
> even if the space is empty and there is no pending transactions
> affecting it. This prevents us from implementing transactional DDL.
> Let's call wal_sync() only if there's actually at least one pending
> transaction affecting the altered space and waiting for WAL.
> ---
>  src/box/vinyl.c | 51 ++++++++++++++++++++++++++++++---------------------
>  src/box/vy_tx.c | 11 ++++++++++-
>  src/box/vy_tx.h |  7 ++++++-
>  3 files changed, 46 insertions(+), 23 deletions(-)
> 
> diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> index 128b1199..e0de65d0 100644
> --- a/src/box/vinyl.c
> +++ b/src/box/vinyl.c
> @@ -1052,25 +1052,8 @@ vinyl_space_invalidate(struct space *space)
>  	 * soon as it's done reading disk, which will make the DML
>  	 * request bail out early, without dereferencing the space.
>  	 */
> -	tx_manager_abort_writers_for_ddl(env->xm, space);
> -}

Please add a comment for vinyl_space_invalidate, explaining what
it is used for and who's using it, just like in the comment that
you removed:
> -
> -/**
> - * This function is called after installing on_replace trigger
> - * used for propagating changes done during DDL. It aborts all
> - * rw transactions affecting the given space that began
> - * before the trigger was installed so that DDL doesn't miss
> - * their working set.
> - */
> -static void
> -vy_abort_writers_for_ddl(struct vy_env *env, struct space *space)

Otherwise LGTM.

-- 
Konstantin Osipov, Moscow, Russia