From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 E0B7E4696C3 for ; Fri, 10 Apr 2020 18:13:14 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <3595ddfb-4635-61b8-97ae-105f9994087d@tarantool.org> Date: Fri, 10 Apr 2020 17:13:12 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Nikita Pettik , tarantool-patches@dev.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() > + | --- > + | ...