From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 27A0F42F4AD for ; Sat, 20 Jun 2020 19:33:13 +0300 (MSK) Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1jmgQa-0005Yj-EI for tarantool-patches@dev.tarantool.org; Sat, 20 Jun 2020 19:33:12 +0300 References: <50a25fbae907f1b0d5406fb2e78d40d3e42a8a8d.1591029888.git.korablev@tarantool.org> <4a83c68d-2dfd-23a8-97cd-5a429639dc3c@tarantool.org> <20200619122412.GA19725@tarantool.org> From: Vladislav Shpilevoy Message-ID: <7b3d6365-5d32-7719-f416-0015782802d6@tarantool.org> Date: Sat, 20 Jun 2020 18:33:11 +0200 MIME-Version: 1.0 In-Reply-To: <20200619122412.GA19725@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2] vinyl: restart read iterator in case of rolled back WAL List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org On 19/06/2020 14:24, Nikita Pettik wrote: > On 16 Jun 15:10, Aleksandr Lyapunov wrote: >> Thanks for the patch! See my 2 comments below: >> >> On 6/1/20 7:46 PM, Nikita Pettik wrote: >>> + if (vy_mem_tree_iterator_is_invalid(&src->mem_iterator.curr_pos)) { >>> + assert(src->mem_iterator.curr.stmt == NULL); >>> + return 1; >>> + } >> I'm afraid that the iterator will not always be invalid in the given case. >> As I see, if a mem holds any older tuple with the same key (e.g. older >> version of that tuple), the restoration routine will find the older tuple >> with the non-invalid iterator. >> I also think that mem_restore must handle all the possible data >> changes by itself without concern of read_iterator. > > You are likely to be right, but I followed suggestion below > to simplify resotration procedure. What is a problem if the older tuple is returned? This is the expected behaviour, isn't it? >>> - if (vy_read_iterator_restore_mem(itr, &next) != 0) >>> + int rc = vy_read_iterator_restore_mem(itr, &next); >>> + if (rc < 0) >>> return -1; >>> + if (rc > 0) { >>> + vy_read_iterator_restore(itr); >>> + goto restart; >>> + } > diff --git a/src/box/vy_read_iterator.c b/src/box/vy_read_iterator.c > index 62a8722d9..409796910 100644 > --- a/src/box/vy_read_iterator.c > +++ b/src/box/vy_read_iterator.c > @@ -536,10 +477,8 @@ rescan_disk: > * as it is owned exclusively by the current fiber so the only > * source to check is the active in-memory tree. > */ > - int rc = vy_read_iterator_restore_mem(itr, &next); > - if (rc < 0) > - return -1; > - if (rc > 0) { > + struct vy_mem_iterator *mem_itr = &itr->src[itr->mem_src].mem_iterator; > + if (mem_itr->version != mem_itr->mem->version) { > vy_read_iterator_restore(itr); > goto restart; I don't like this solution, because it is easy to change mem version while it was used in a read iterator. Previously it was solved without returning back to disk, so the iterator couldn't "infinitely" follow the disk <-> mem cycle, when lots of updates land to this mem. Now it can. I tend to agree here with Kostja. I don't understand why should we restart the whole iterator when only mem has changed. This looks like a regression. However my LGTM is not necessary here, after Alexander L. gave it. So I am not going to insist on anything.