From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Wed, 29 May 2019 18:58:32 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH v3 1/2] memtx: add yields during index build
Message-ID: <20190529155832.tmbbxknlyt72j7ym@esperanza>
References: <cover.1559056498.git.sergepetrenko@tarantool.org>
 <398dbcffaa3254d81e8b77a93a541ce26fa8f50d.1559056498.git.sergepetrenko@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <398dbcffaa3254d81e8b77a93a541ce26fa8f50d.1559056498.git.sergepetrenko@tarantool.org>
To: Serge Petrenko <sergepetrenko@tarantool.org>
Cc: tarantool-patches@freelists.org, kostja@tarantool.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Tue, May 28, 2019 at 06:33:24PM +0300, Serge Petrenko wrote:
> diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> index 5ddb4f7ee..7a3e6b74a 100644
> --- a/src/box/memtx_space.c
> +++ b/src/box/memtx_space.c
> @@ -933,8 +1005,38 @@ memtx_space_build_index(struct space *src_space, struct index *new_index,
>  		 */
>  		if (new_index->def->iid == 0)
>  			tuple_ref(tuple);
> +		if (++count % YIELD_LOOPS == 0) {
> +			/*
> +			 * Remember the latest inserted tuple to
> +			 * avoid processing yet to be added tuples
> +			 * in on_replace triggers.
> +			 */
> +			state.cursor = tuple;

I think that state.cursor should be referenced counted - otherwise it
might get freed right under us.

> +			fiber_sleep(0);
> +			/*
> +			 * The on_replace trigger may have failed
> +			 * during the yield.
> +			 */
> +			if (state.rc != 0) {
> +				rc = -1;
> +				diag_move(&state.diag, diag_get());
> +				break;
> +			}
> +		}
> +		/*
> +		 * Sleep after at least 1 tuple is inserted to test
> +		 * on_replace triggers for index build.
> +		 * Sleep only once.
> +		 */
> +		struct errinj *inj = errinj(ERRINJ_BUILD_INDEX_DELAY, ERRINJ_DOUBLE);
> +		if (inj != NULL && inj->dparam > 0 && count == 1) {
> +			state.cursor = tuple;
> +			fiber_sleep(inj->dparam);
> +		}

The error injection should be added in the next patch, the one that adds
tests. Please re-split.

Other than that, looks good to me.