Re: [Tarantool-patches] [PATCH v2] vinyl: restart read iterator in case of rolled back WAL

[Nikita Pettik <korablev@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;
        }