Re: [Tarantool-patches] [PATCH 2/2] vinyl: skip invalid upserts during squash

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

> diff --git a/src/box/vy_history.c b/src/box/vy_history.c
> index 0f3b71195..2232348f9 100644
> --- a/src/box/vy_history.c
> +++ b/src/box/vy_history.c
> @@ -102,12 +102,22 @@ vy_history_apply(struct vy_history *history, struct key_def *cmp_def,
>  	while (node != NULL) {
>  		struct tuple *stmt = vy_apply_upsert(node->stmt, curr_stmt,
>  						     cmp_def, format, true);
> -		++*upserts_applied;
> -		if (curr_stmt != NULL)
> -			tuple_unref(curr_stmt);
> -		if (stmt == NULL)
> -			return -1;
> -		curr_stmt = stmt;
> +		if (stmt == NULL) {
> +			/*
> +			 * In case statement hasn't been applied,
> +			 * simply skip it ignoring errors (otherwise
> +			 * error will appear during tuple read).
> +			 */
> +			assert(diag_last_error(diag_get()) != NULL);
> +			struct error *e = diag_last_error(diag_get());
> +			if (e->type != &type_ClientError)
> +				return -1;

*. Shouldn't we log here?

> diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
> index 04c9926a8..3d630fc01 100644
> --- a/src/box/vy_lsm.c
> +++ b/src/box/vy_lsm.c
> @@ -984,13 +984,20 @@ vy_lsm_commit_upsert(struct vy_lsm *lsm, struct vy_mem *mem,
>  		struct tuple *upserted =
>  			vy_apply_upsert(stmt, older, lsm->cmp_def,
>  					lsm->mem_format, false);
> -		lsm->stat.upsert.applied++;
> -
>  		if (upserted == NULL) {
> -			/* OOM */
> +			/*
> +			 * OOM or upserted tuple does not fulfill
> +			 * space format. Hence, upsert can't be
> +			 * transformed into replace. Log error
> +			 * and exit.
> +			 */
> +			struct error *e = diag_last_error(diag_get());
> +			assert(e != NULL);
> +			error_log(e);

*. These three lines can be replaced with diag_log();.

> diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
> index 5029bd8a1..1a2ea43d9 100644
> --- a/src/box/vy_tx.c
> +++ b/src/box/vy_tx.c
> @@ -515,11 +515,11 @@ vy_tx_write(struct vy_lsm *lsm, struct vy_mem *mem,
>  						    region_stmt);
>  				tuple_unref(applied);
>  				return rc;
> +			} else {
> +				struct error *e = diag_last_error(diag_get());
> +				assert(e != NULL);
> +				error_log(e);

*. The same here.

> diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c
> index 7a6a20627..4d34d96c8 100644
> --- a/src/box/vy_write_iterator.c
> +++ b/src/box/vy_write_iterator.c
> @@ -850,10 +850,23 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
>  		       vy_stmt_type(hint) != IPROTO_UPSERT);
>  		struct tuple *applied = vy_apply_upsert(h->tuple, hint,
>  				stream->cmp_def, stream->format, false);
> -		if (applied == NULL)
> -			return -1;
> -		vy_stmt_unref_if_possible(h->tuple);
> -		h->tuple = applied;
> +		if (applied == NULL) {
> +			/*
> +			 * Current upsert can't be applied.
> +			 * Let's skip it and log error.
> +			 */
> +			struct error *e = diag_last_error(diag_get());
> +			assert(e != NULL);
> +			if (e->type != &type_ClientError)
> +				return -1;
> +			say_info("upsert %s is not applied due to the error: %s",
> +				 vy_stmt_str(h->tuple), e->errmsg);
> +			vy_stmt_unref_if_possible(h->tuple);

*. Why here we use sometimes say_info() and sometimes error_log()? Why
not something one?

> @@ -864,10 +877,17 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint,
>  		assert(result->tuple != NULL);
>  		struct tuple *applied = vy_apply_upsert(h->tuple, result->tuple,
>  					stream->cmp_def, stream->format, false);
> -		if (applied == NULL)
> -			return -1;
> -		vy_stmt_unref_if_possible(result->tuple);
> -		result->tuple = applied;
> +		if (applied == NULL) {
> +			struct error *e = diag_last_error(diag_get());
> +			assert(e != NULL);
> +			if (e->type != &type_ClientError)
> +				return -1;
> +			say_info("upsert %s is not applied due to the error: %s",
> +				 vy_stmt_str(h->tuple), e->errmsg);
> +		} else {
> +			vy_stmt_unref_if_possible(result->tuple);
> +			result->tuple = applied;
> +		}
>  		vy_stmt_unref_if_possible(h->tuple);
>  		/*
>  		 * Don't bother freeing 'h' since it's