[Tarantool-patches] [PATCH 1/2] vinyl: init all vars before cleanup in vy_lsm_split_range()
Nikita Pettik
korablev at tarantool.org
Fri Apr 10 18:40:50 MSK 2020
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()
> > + | ---
> > + | ...
More information about the Tarantool-patches
mailing list