From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id D6ED04696C3 for ; Fri, 10 Apr 2020 18:40:51 +0300 (MSK) Date: Fri, 10 Apr 2020 15:40:50 +0000 From: Nikita Pettik Message-ID: <20200410154050.GD9428@tarantool.org> References: <3595ddfb-4635-61b8-97ae-105f9994087d@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <3595ddfb-4635-61b8-97ae-105f9994087d@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 1/2] vinyl: init all vars before cleanup in vy_lsm_split_range() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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() > > + | --- > > + | ...