From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 8162E42F4AD for ; Fri, 19 Jun 2020 15:24:13 +0300 (MSK) Date: Fri, 19 Jun 2020 12:24:12 +0000 From: Nikita Pettik Message-ID: <20200619122412.GA19725@tarantool.org> References: <50a25fbae907f1b0d5406fb2e78d40d3e42a8a8d.1591029888.git.korablev@tarantool.org> <4a83c68d-2dfd-23a8-97cd-5a429639dc3c@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <4a83c68d-2dfd-23a8-97cd-5a429639dc3c@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: Aleksandr Lyapunov Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org 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? Ok, I'm fine with it. Consider this change: 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; }