Re: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/vy_scheduler.c b/src/box/vy_scheduler.c
> index 9dba93d34..387f58723 100644
> --- a/src/box/vy_scheduler.c
> +++ b/src/box/vy_scheduler.c
> @@ -1065,8 +1065,10 @@ vy_task_write_run(struct vy_task *task, bool no_compression)
>  				 no_compression) != 0)
>  		goto fail;
>  
> -	if (wi->iface->start(wi) != 0)
> +	if (wi->iface->start(wi) != 0) {
> +		wi->iface->stop(wi);

1. I would better make start() more self-sufficient. Otherwise it
failed to start, and yet you somewhy need to stop it. Looks confusing.

>  		goto fail_abort_writer;
> +	}
> diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> index af116a4b4..ea8dce0ba 100644
> --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> @@ -242,6 +242,91 @@ s:drop()
> +assert(s.index.pk:stat().range_count == 1)
> + | ---
> + | - true
> + | ...
> +assert(s.index.pk:stat().run_count == 2)
> + | ---
> + | - true
> + | ...
> +
> +errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', true)
> + | ---
> + | - ok
> + | ...
> +-- Prevent next attempt to compact in a row.
> +--
> +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 1)
> + | ---
> + | - ok
> + | ...
> +
> +s.index.pk:compact()
> + | ---
> + | ...
> +-- Leave a time gap between compaction and index drop just in case
> +-- (to make sure that compaction is already finished (re-scheduled)
> +--  when at the moment of index drop).
> +--
> +fiber.sleep(0.5)

2. Can't you wait for compaction actively on some condition? Such as
smaller run count. Half of second is quite a big timeout for a regular
test.

> + | ---
> + | ...
> +
> +-- Drop is required to unref all tuples.
> +--
> +s:drop()
> + | ---
> + | ...
> +-- After index is dropped, not all tuples are deallocated at once:
> +-- they may be still referenced (while being pushed) in Lua. So
> +-- invoke GC explicitly.
> +--
> +collectgarbage("collect")
> + | ---
> + | - 0
> + | ...
> +-- Give GC some time to operate on.
> +--
> +fiber.sleep(1)

3. GC is synchronous. So if collectgarbage() has returned, GC is done.

> + | ---
> + | ...
> +