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

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

Hi! Thanks for the patch!

Firstly, Kostja left some comments here. Would be cool to address them.

Secondly, here is my personal opinion. I don't like just skipping things
a user committed without any error appearing in the application. IMO, we
should apply only the first commit. And let a user see this error so as he
could notice the problem. To fix reads he could do delete() of the bad key.

However, how a user will be able to find the exact broken key - I don't
know. Maybe the ignore + logging is better.

On 13/04/2020 23:55, Nikita Pettik wrote:
> Instead of aborting merge sequence of upserts let's log appeared
> errors and skip upserts which can't be applied. It makes sense
> taking into consideration previous commit: now upsert statements which
> can't be applied may appear in mems/runs (previously squash operation
> might fail only due to OOM). As a result, if we didn't ignore invalid
> upserts, dump or compaction processes would not be able to finish (owing
> to inability to squash upserts).
> 
> Closes #1622
> ---
> diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
> index 5029bd8a1..060a7f6a9 100644
> --- a/src/box/vy_tx.c
> +++ b/src/box/vy_tx.c
> @@ -515,11 +515,15 @@ vy_tx_write(struct vy_lsm *lsm, struct vy_mem *mem,
>  						    region_stmt);
>  				tuple_unref(applied);
>  				return rc;
> +			} else {
> +				/*
> +				 * Ignore a memory error, because it is
> +				 * not critical to apply the optimization.
> +				 * Clear diag: otherwise error is set but
> +				 * function may return success return code.
> +				 */
> +				diag_clear(diag_get());

Why do you clear it? Diagnostics area is usually not cleared (at least
in application code), and contains some last happened error. In C code we
anyway use result value of a function to determine its result.