[tarantool-patches] Re: [PATCH 09/12] vinyl: consolidate skip optimization checks in read iterator

[Vladislav Shpilevoy]

Hello. I see several unsettling problems here.

1. Old skip functions had returned their last_stmt (itr->last_stmt), that
updates their current statement (itr->curr_stmt). This was done if an iterator
is not 'behind' according to the new terminology. But now if an iterator is
behind, it does nothing and does not update itr->curr_stmt as well.

2. Old vy_cache_iterator_skip in the non-behind state could set 'stop'
flag triggering skipped_src propagation, but now if cache is not behind,
stop flag is always false, and skipped_src is not propagated.

3. vy_read_iterator_scan_txw does not use vy_read_src_is_behind() - it
continues old way usage:
if (!src->is_started)
	vy_txw_iterator_skip(src_itr, itr->last_stmt, &src->stmt);

On 15/04/2018 22:55, Vladimir Davydov wrote:
> Each kind of source iterator has 'skip' method, which is called to
> position the iterator to a specified key. It is used to advance
> sources that were skipped on the previous iteration (e.g. because
> the result was found in the cache). The method has an optimization:
> it avoids a lookup in the index if it is already positioned at a
> statement following the specified key. This optimization makes the
> method difficult to use if we want to keep a key history in each
> source instead of a single statement, as we don't know whether 'skip'
> changed the position or not and hence whether we need to rebuild key
> history or not. Let's move the optimization to the read iterator and
> make 'skip' plain and simple: let it always reposition the iterator
> to the first statement following a given key.
> ---
>   src/box/vy_cache.c         | 19 -------------------
>   src/box/vy_mem.c           | 14 --------------
>   src/box/vy_read_iterator.c | 23 ++++++++++++++++++++---
>   src/box/vy_run.c           | 13 -------------
>   src/box/vy_tx.c            | 14 --------------
>   5 files changed, 20 insertions(+), 63 deletions(-)
>