Re: [PATCH v3 1/2] memtx: add yields during index build

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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.