Re: [Tarantool-patches] [PATCH 1/2] vinyl: init all vars before cleanup in vy_lsm_split_range()

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

Hi! Thanks for the patch!

See 3 comments below.

> diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
> index 3d3f41b7a..04c9926a8 100644
> --- a/src/box/vy_lsm.c
> +++ b/src/box/vy_lsm.c
> @@ -134,6 +134,11 @@ vy_stmt_alloc(struct tuple_format *format, uint32_t bsize)
>  {
>  	uint32_t total_size = sizeof(struct vy_stmt) + format->field_map_size +
>  		bsize;
> +	struct errinj *inj = errinj(ERRINJ_VY_MAX_TUPLE_SIZE, ERRINJ_INT);
> +	if (inj != NULL && inj->iparam >= 0) {
> +		if (inj->iparam-- == 0)

1. You set ERRINJ_VY_MAX_TUPLE_SIZE to an integer. Why not to a boolean,
which would set it to false instead of decrement? That would make it
clear the injection works only once.

Also it looks too artificial. The injection basically simulates a tuple
with too big size which was inserted bypassing max_tuple_size check,
and suddenly it was checked here, already after insertion.

Better add an OOM injection for malloc a few lines below, would be more
correct.

> +			total_size = vy_max_tuple_size + 1;
> +	}
>  	if (unlikely(total_size > vy_max_tuple_size)) {
>  		diag_set(ClientError, ER_VINYL_MAX_TUPLE_SIZE,
>  			 (unsigned) total_size);
> diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> new file mode 100644
> index 000000000..2c03697f6
> --- /dev/null
> +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result
> @@ -0,0 +1,93 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +fiber = require('fiber')
> + | ---
> + | ...
> +digest = require('digest')
> + | ---
> + | ...
> +
> +s = box.schema.space.create('test', {engine = 'vinyl'})
> + | ---
> + | ...
> +_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
> + | ---
> + | ...
> +
> +test_run:cmd("setopt delimiter ';'")
> + | ---
> + | - true
> + | ...
> +function dump(big)
> +    local step = big and 1 or 5
> +    for i = 1, 20, step do
> +        s:replace{i, digest.urandom(1000)}
> +    end
> +    box.snapshot()
> +end;
> + | ---
> + | ...
> +
> +function compact()
> +    s.index.pk:compact()
> +    repeat
> +        fiber.sleep(0.001)
> +        local info = s.index.pk:stat()
> +    until info.range_count == info.run_count
> +end;
> + | ---
> + | ...
> +test_run:cmd("setopt delimiter ''");
> + | ---
> + | - true
> + | ...
> +
> +-- The first run should be big enough to prevent major compaction
> +-- on the next dump, because run_count_per_level is ignored on the
> +-- last level.
> +--
> +dump(true)
> + | ---
> + | ...
> +dump()
> + | ---
> + | ...
> +-- 1 range, 2 runs

2. Can you add an assertion, that it is really 1 range and 2 runs?
To be sure that the test won't become useless in future if suddenly
something will change there.

> +
> +compact()
> + | ---
> + | ...
> +-- 1 range, 1 run after compaction
> +
> +dump()
> + | ---
> + | ...
> +-- 1 range, 2 runs
> +
> +errinj = box.error.injection
> + | ---
> + | ...
> +errinj.set('ERRINJ_VY_MAX_TUPLE_SIZE', 0)
> + | ---
> + | - ok
> + | ...
> +-- Should finish successfully despite vy_stmt_alloc() fail.
> +--
> +compact()
> + | ---
> + | ...
> +-- 1 range, 1 run
> +s.index.pk:stat().range_count
> + | ---
> + | - 1
> + | ...
> +s.index.pk:stat().run_count

3. I would add an assertion/print, that ERRINJ_VY_MAX_TUPLE_SIZE
(or whatever will be used after the review) is changed back to
off, and then change it to off again anyway. Because if the test
won't hit the errinj because of something in future, the errinj
will be left set, and we won't notice that here. Also it will
make all next vinyl tests running in the same job fail in very
surprising ways.

The same for the next commit.

> + | ---
> + | - 1
> + | ...
> +
> +s:drop()
> + | ---
> + | ...