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

[Nikita Pettik <korablev@tarantool.org>]

On 01 May 02:31, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> Firstly, Kostja left some comments here. Would be cool to address them.

Done (sorry, I did not ignore them, just had to work on other more vital bugs).
 
> 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.

The problem with delete it leaves user no way to restore the rest
of upsert history. Moreover, these upserts will get stuck until
user finds in logs corresponding error (I guess we can't abort
compaction due to invalid upserts).

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

Why can't we just log broken key? E.g. see logs in vy_apply_upsert().

> 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.

Agree, forgot that we do not erase diag before each request execution.
Removed this clean-up.