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

[Nikita Pettik]

On 29 May 23:47, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> >>> 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?
> > 
> > Indeed, let's use here say_error():
> 
> Now in some places we use diag_log() and in other say_error() + vy_stmt_str().
> How is it chosen when what to use?


This place (vy_read_view_merge()) is called only during dump and compaction
processing. Logging tuple itself would significantly simply diagnostics.
Meanwhile in other places diag_log() fires once wrong tuple is inserted.
Btw, Konstantin agreed with this logging way:

>* Nikita Pettik <korablev at tarantool.org> [20/05/14 09:50]:
> On 14 Apr 01:12, Konstantin Osipov wrote:
> > * Nikita Pettik <korablev at tarantool.org> [20/04/14 00:57]:
> > > 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).
> >
> > We should log the upsert itself, base64 encoded.
>
> It is logged with following format (in vy_read_view_merge()):
>
> say_info("upsert %s is not applied due to the error: %s",
> vy_stmt_str(h->tuple), e->errmsg);
>
> It is the function which is called during compaction to squash
> upserts.