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

[Konstantin Osipov <kostja.osipov@gmail.com>]

* Nikita Pettik <korablev@tarantool.org> [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