From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 626D64696C3 for ; Fri, 3 Apr 2020 03:21:39 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Fri, 3 Apr 2020 02:21:37 +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] vinyl: bump dump_generation in case scheduler doesn't catch up with DDL 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. On 31/03/2020 02:47, Nikita Pettik wrote: > It may turn out that dump_generation does not catch up with current > generation and no other dump tasks are in progress. This may happen > dump process is throttled due to errors. In this case generation is > bumped but dump_generation is not (since dump is not completed). In > turn, throttling opens a window for DDL operations. For instance, index > dropping and creation of new one results in mentioned situation: > > box.snapshot() -- fails for some reason; next attempt at dumping will be > -- taken in one second. > s:drop() -- drop index to be dumped > s = box.schema.space.create('test', {engine = 'vinyl'}) > -- create new one (its mem generation is greater than scheduler's one) > i = s:create_index('pk') > > Closes #4821 > --- 1. Add a changelog please. > Note that current fix is only workaround amd doesn't pretend to be > the most proper one. Alternatively, we can rollback scheduler > generation in case dump is failed and index to be dumped is dropped. 2. Not sure this is possible. At least I don't see a way. The dump is failed before the index is dropped. And only on a next dump you see that there is a problem. You can try changing the generation in the index drop, if you see, that its generation matches the dump generation. But it looks like encapsulation violation if you do it straight forward. vy_lsm works with the scheduler only via its API. If you go this way, we need a method like vy_scheduler_lsm_is_dropped(). It would check that if the LSM's generation = dump_generation, we should recalculate it not counting this tree. However I am ok with both solutions. Up to you. > Branch: https://github.com/tarantool/tarantool/tree/np/gh-4821-scheduler-gen-assert > Issue: https://github.com/tarantool/tarantool/issues/4821 > > diff --git a/test/vinyl/gh-4821-ddl-during-throttled-dump.result b/test/vinyl/gh-4821-ddl-during-throttled-dump.result > new file mode 100644 > index 000000000..eee031101 3. I rolledback the patch except the test, and it passed anyway. The example in the ticket crashes on my machine, but the test does not. Why not to take the test from the ticket as is? With a small change of throttling timeout using ERRINJ_VY_SCHED_TIMEOUT, to make it fail faster than 1 sec.