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

[Nikita Pettik <korablev@tarantool.org>]

On 10 Apr 17:13, Vladislav Shpilevoy wrote:
> 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.

Cause integer allows setting delay of vy_stmt_alloc() failure.
For instance, I don't want first invocation to vy_stmt_alloc()
fail, but the second, third or tenth one - it may turn out to be
vital. This patch fixes bug when first call of vy_stmt_alloc()
during compaction fails; the next patch - if tenth call of
vy_stmt_alloc() fails.
 
> 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.

Konstantint said, that squashing two upserts of size 'x' may result
in new vy_stmt with size > 'x'. Despite the fact that I did not
attempt at reproducing this statement, I saw these errors appearing
on production machine during compaction. I do not know the exact reason
why they revealed, but it is a fact.
 
> 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);

It makes sense on the one hand, but on another hand - we know that
emulating malloc OOM is likely to be artificial as well.

> > 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.

Ok, will add.
 
> > +
> > +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.

Ok, will fix.

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