[patches] [PATCH 1/1] vinyl: use vinyl iterators virtuality to remove 'if' in next()

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy at tarantool.org> [18/02/17 12:53]:

I agree with the architectural approach idea.

Here's the reason: in SQL, we may have batch-next method of the
iterator, which will be completely different in primary and
secondary indexes. So it's Ok to split secondary and primary index
already now.

Vladimir, please take a look at this patch and I will push if
you're OK.

> The first reason of the patch is that vinyl iterators are virtual
> already, and 'if's about constant index attributes (like index->id)
> can be replaced by new next() implementation. Now in next() index->id
> is checked to detect necessity of primary index lookup.
> 
> Lets split next() in 2 functions: primary_next() and secondary_next()
> to remove 'if'.
> 
> The second reason, that in #2129 logic of secondary index lookup
> complicates a lot. For example, there is raw idea to do not add
> statements into a cache before looking up in a primary index, because
> after #2129 any tuple, read from a secondary index, can be dirty.
> 
> Needed for #2129
> 
> Signed-off-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> ---
>  src/box/vinyl.c | 55 +++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> index 03ad2bf4a..992ceca37 100644
> --- a/src/box/vinyl.c
> +++ b/src/box/vinyl.c
> @@ -3823,10 +3823,11 @@ vinyl_iterator_close(struct vinyl_iterator *it)
>  }
>  
>  static int
> -vinyl_iterator_next(struct iterator *base, struct tuple **ret)
> +vinyl_iterator_primary_next(struct iterator *base, struct tuple **ret)
>  {
> -	assert(base->next = vinyl_iterator_next);
> +	assert(base->next = vinyl_iterator_primary_next);
>  	struct vinyl_iterator *it = (struct vinyl_iterator *)base;
> +	assert(it->index->id == 0);
>  	struct tuple *tuple;
>  
>  	if ((*it->rv)->is_aborted ||
> @@ -3843,21 +3844,44 @@ vinyl_iterator_next(struct iterator *base, struct tuple **ret)
>  		*ret = NULL;
>  		return 0;
>  	}
> +	*ret = tuple_bless(tuple);
> +	if (*ret != NULL)
> +		return 0;
> +fail:
> +	vinyl_iterator_close(it);
> +	return -1;
> +}
>  
> -	if (it->index->id > 0) {
> -		/* Get the full tuple from the primary index. */
> -		if (vy_index_get(it->index->pk, it->tx, it->rv,
> -				 tuple, &tuple) != 0)
> -			goto fail;
> -	} else {
> -		tuple_ref(tuple);
> +static int
> +vinyl_iterator_secondary_next(struct iterator *base, struct tuple **ret)
> +{
> +	assert(base->next = vinyl_iterator_secondary_next);
> +	struct vinyl_iterator *it = (struct vinyl_iterator *)base;
> +	assert(it->index->id > 0);
> +	struct tuple *tuple;
> +
> +	if ((*it->rv)->is_aborted ||
> +	    (it->tx != NULL && it->tx->state == VINYL_TX_ABORT)) {
> +		goto fail;
>  	}
> -	*ret = tuple_bless(tuple);
> -	tuple_unref(tuple);
> -	if (*ret == NULL)
> +
> +	if (vy_read_iterator_next(&it->iterator, &tuple) != 0)
>  		goto fail;
>  
> -	return 0;
> +	if (tuple == NULL) {
> +		/* EOF. Close the iterator immediately. */
> +		vinyl_iterator_close(it);
> +		*ret = NULL;
> +		return 0;
> +	}
> +	/* Get the full tuple from the primary index. */
> +	if (vy_index_get(it->index->pk, it->tx, it->rv,
> +			 tuple, &tuple) != 0)
> +		goto fail;
> +	*ret = tuple_bless(tuple);
> +	tuple_unref(tuple);
> +	if (*ret != NULL)
> +		return 0;
>  fail:
>  	vinyl_iterator_close(it);
>  	return -1;
> @@ -3916,7 +3940,10 @@ vinyl_index_create_iterator(struct index *base, enum iterator_type type,
>  		goto err_key;
>  
>  	iterator_create(&it->base, base);
> -	it->base.next = vinyl_iterator_next;
> +	if (index->id == 0)
> +		it->base.next = vinyl_iterator_primary_next;
> +	else
> +		it->base.next = vinyl_iterator_secondary_next;
>  	it->base.free = vinyl_iterator_free;
>  
>  	it->env = env;
> -- 
> 2.14.3 (Apple Git-98)

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.org - www.twitter.com/kostja_osipov