Re: [Tarantool-patches] [PATCH v3 1/2] vinyl: rework upsert operation

[Nikita Pettik <korablev@tarantool.org>]

On 07 Oct 00:12, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 3 comments below, and my fixes on top of this branch in a
> separate commit.
> 
> >  src/box/vinyl.c                 |   2 +-
> >  src/box/vy_stmt.c               |  28 +-
> >  src/box/vy_stmt.h               |   5 +-
> >  src/box/vy_upsert.c             | 302 +++++++++++---------
> >  test/unit/vy_iterators_helper.c |   2 +-
> >  test/vinyl/upsert.result        | 473 ++++++++++++++++++++++++++++++++
> >  test/vinyl/upsert.test.lua      | 194 +++++++++++++
> >  7 files changed, 867 insertions(+), 139 deletions(-)
> > 
> > diff --git a/src/box/vinyl.c b/src/box/vinyl.c
> > index cee39c58c..9d9ee0613 100644
> > --- a/src/box/vinyl.c
> > +++ b/src/box/vinyl.c
> > @@ -1976,7 +1976,7 @@ vy_lsm_upsert(struct vy_tx *tx, struct vy_lsm *lsm,
> >  	operations[0].iov_base = (void *)expr;
> >  	operations[0].iov_len = expr_end - expr;
> >  	vystmt = vy_stmt_new_upsert(lsm->mem_format, tuple, tuple_end,
> > -				    operations, 1);
> > +				    operations, 1, false);
> 
> 1. You don't really need that flag. In the previous review I tried to
> explain it, but now I added a commit on top of your branch where I
> removed this parameter. Take a look.
> 
> This argument does not have much sense. First of all, the operations array
> is not really operations. Iovecs here can be separate operations; it can be
> a single iovec with all operations; it can be several iovecs with first
> having additional MP_ARRAY encoded before a first operation. So these are
> just pieces of raw data. And you can use that. You can push a header of
> these operations as a part of 'operations' array always, like I did in my
> commit. No need to pass a new argument for that.

Ok, thx, squashed.
 
> >  	if (vystmt == NULL)
> >  		return -1;
> >  	assert(vy_stmt_type(vystmt) == IPROTO_UPSERT);
> > diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c
> > index e697b6321..2b5a16730 100644
> > --- a/src/box/vy_upsert.c
> > +++ b/src/box/vy_upsert.c
> > @@ -39,39 +39,150 @@
> >  #include "column_mask.h"
> >  
> > +/**
> > + * Apply update operations stored in @a upsert on tuple @a stmt. If @a stmt is
> > + * void statement (i.e. it is NULL or delete statement) then operations are
> > + * applied on tuple stored in @a upsert. Update operations of @a upsert which
> 
> 2. Applied on tuple stored in @a upsert? Upsert logic is that the operations are
> ignored, if it appears the statement is first in its key. Did you mean, that you
> apply all operation sets except the first set? Below I see a comment
> 
> 	/*
> 	 * In case upsert folds into insert, we must skip first
> 	 * update operations.
> 	 */
> 
> So this is probably it. Just the function comment is a bit misleading.

A bit fixed to this version:

 * Apply update operations from @a upsert on tuple @a stmt.
 
> > + * can't be applied are skipped along side with other operations from single
> > + * group (i.e. packed in one msgpack array); errors may be logged depending on
> > + * @a suppress_error flag.
> >   *
> > - * @retval 0 && *result_stmt != NULL : successful squash
> > - * @retval 0 && *result_stmt == NULL : unsquashable sources
> > - * @retval -1 - memory error
> > + * @param upsert Upsert statement to be applied on @a stmt.
> > + * @param stmt Statement to be used as base for upsert operations.
> > + * @param cmp_def Key definition required to provide check of primary key
> > + *                modification.
> 
> 3. param suppress_error is missing.

Added description:

 * @param suppress_error If true, do not raise/log any errors.

> 
> > + * @return Tuple containing result of upsert application; NULL in case OOM.
> >   */