From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f65.google.com (mail-lf1-f65.google.com [209.85.167.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B382B42EF5C for ; Fri, 19 Jun 2020 15:42:31 +0300 (MSK) Received: by mail-lf1-f65.google.com with SMTP id g139so4662057lfd.10 for ; Fri, 19 Jun 2020 05:42:31 -0700 (PDT) Date: Fri, 19 Jun 2020 15:42:29 +0300 From: Konstantin Osipov Message-ID: <20200619124229.GB61079@atlas> References: <50a25fbae907f1b0d5406fb2e78d40d3e42a8a8d.1591029888.git.korablev@tarantool.org> <4a83c68d-2dfd-23a8-97cd-5a429639dc3c@tarantool.org> <20200619122412.GA19725@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200619122412.GA19725@tarantool.org> 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: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org * Nikita Pettik [20/06/19 15:26]: > 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. > > > > - 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; > > > + } > > The read iterator was rewritten several times and still have at least > > several bugs. I think we should admit that we cannot support such a > > complicated solution. How about some stupid solution: if ANY change > > has been happened during yield - restart advancing? Does this statement have technical merit? Is it supported by tests? I'd gladly support the change if it was grounded in reason - evaluation of the performance impact, for example, could serve as a confirmation that a simple solution would be just fine. Without it, I'd it's a regress, a signature of helplessness, lack of courage to make things right. > > 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 > @@ -378,65 +378,6 @@ vy_read_iterator_scan_disk(struct vy_read_iterator *itr, uint32_t disk_src, > return 0; > } > > -/** > - * Restore the position of the active in-memory tree iterator > - * after a yield caused by a disk read and update 'next' > - * if necessary. > - * @retval -1 In case of error (e.g. OOM); > - * @retval 0 Successful execution; > - * @retval 1 Restart of advance_iterator is required. > - */ > -static NODISCARD int > -vy_read_iterator_restore_mem(struct vy_read_iterator *itr, > - struct vy_entry *next) > -{ > - int rc; > - int cmp; > - struct vy_read_src *src = &itr->src[itr->mem_src]; > - > - rc = vy_mem_iterator_restore(&src->mem_iterator, > - itr->last, &src->history); > - if (rc < 0) > - return -1; /* memory allocation error */ > - if (rc == 0) > - return 0; /* nothing changed */ > - > - if (vy_mem_tree_iterator_is_invalid(&src->mem_iterator.curr_pos)) { > - assert(src->mem_iterator.curr.stmt == NULL); > - return 1; > - } > - struct vy_entry entry = vy_history_last_stmt(&src->history); > - cmp = vy_read_iterator_cmp_stmt(itr, entry, *next); > - if (cmp > 0) { > - /* > - * Memory trees are append-only so if the > - * source is not on top of the heap after > - * restoration, it was not before. > - */ > - assert(src->front_id < itr->front_id); > - return 0; > - } > - if (cmp < 0) { > - /* > - * The new statement precedes the current > - * candidate for the next key. > - */ > - *next = entry; > - itr->front_id++; > - } else { > - /* > - * The new statement updates the next key. > - * Make sure we don't read the old value > - * from the cache while applying UPSERTs. > - */ > - struct vy_read_src *cache_src = &itr->src[itr->cache_src]; > - if (cache_src->front_id == itr->front_id) > - vy_history_cleanup(&cache_src->history); > - } > - src->front_id = itr->front_id; > - return 0; > -} > - > static void > vy_read_iterator_restore(struct vy_read_iterator *itr); > > @@ -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; > } > -- Konstantin Osipov, Moscow, Russia