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

[Vladislav Shpilevoy <v.shpilevoy@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.