From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 836A74696C3 for ; Fri, 1 May 2020 03:55:42 +0300 (MSK) References: <7965217ceed66d448cee453c690e8d91ba7a841b.1587948306.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <19283eae-5eb9-3edb-2dea-8588874e174b@tarantool.org> Date: Fri, 1 May 2020 02:55:40 +0200 MIME-Version: 1.0 In-Reply-To: <7965217ceed66d448cee453c690e8d91ba7a841b.1587948306.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v3 3/3] vinyl: clean-up write iterator if vy_task_write_run() fails 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_scheduler.c b/src/box/vy_scheduler.c > index 9dba93d34..387f58723 100644 > --- a/src/box/vy_scheduler.c > +++ b/src/box/vy_scheduler.c > @@ -1065,8 +1065,10 @@ vy_task_write_run(struct vy_task *task, bool no_compression) > no_compression) != 0) > goto fail; > > - if (wi->iface->start(wi) != 0) > + if (wi->iface->start(wi) != 0) { > + wi->iface->stop(wi); 1. I would better make start() more self-sufficient. Otherwise it failed to start, and yet you somewhy need to stop it. Looks confusing. > goto fail_abort_writer; > + } > diff --git a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result > index af116a4b4..ea8dce0ba 100644 > --- a/test/vinyl/gh-4864-stmt-alloc-fail-compact.result > +++ b/test/vinyl/gh-4864-stmt-alloc-fail-compact.result > @@ -242,6 +242,91 @@ s:drop() > +assert(s.index.pk:stat().range_count == 1) > + | --- > + | - true > + | ... > +assert(s.index.pk:stat().run_count == 2) > + | --- > + | - true > + | ... > + > +errinj.set('ERRINJ_VY_WRITE_ITERATOR_START_FAIL', true) > + | --- > + | - ok > + | ... > +-- Prevent next attempt to compact in a row. > +-- > +errinj.set("ERRINJ_VY_SCHED_TIMEOUT", 1) > + | --- > + | - ok > + | ... > + > +s.index.pk:compact() > + | --- > + | ... > +-- Leave a time gap between compaction and index drop just in case > +-- (to make sure that compaction is already finished (re-scheduled) > +-- when at the moment of index drop). > +-- > +fiber.sleep(0.5) 2. Can't you wait for compaction actively on some condition? Such as smaller run count. Half of second is quite a big timeout for a regular test. > + | --- > + | ... > + > +-- Drop is required to unref all tuples. > +-- > +s:drop() > + | --- > + | ... > +-- After index is dropped, not all tuples are deallocated at once: > +-- they may be still referenced (while being pushed) in Lua. So > +-- invoke GC explicitly. > +-- > +collectgarbage("collect") > + | --- > + | - 0 > + | ... > +-- Give GC some time to operate on. > +-- > +fiber.sleep(1) 3. GC is synchronous. So if collectgarbage() has returned, GC is done. > + | --- > + | ... > +