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

[Nikita Pettik <korablev@tarantool.org>]

On 14 Oct 14:47, Aleksandr Lyapunov wrote:
> Great, LGTM!

Pushed to master, changelog is updated, branch is dropped.
 
> On 10/14/20 1:42 PM, Nikita Pettik wrote:
> > On 14 Oct 11:58, Aleksandr Lyapunov wrote:
> > > >           if (stmt_is_void) {
> > > >                   ups_cnt--;
> > > >                   mp_next(&ups_ops);
> > > > +               stmt = vy_stmt_new_replace(tuple_format(upsert), result_mp,
> > > > +                                          result_mp_end);
> > > Why can't you just use `upsert` tuple itself instead of created tuple by its
> > > format and data?
> > > 
> > For sure we can. Sorry it was late night so I didn't pay attention to this fact.
> > Diff:
> > 
> > diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c
> > index 0b4baf6ff..fdae931f6 100644
> > --- a/src/box/vy_upsert.c
> > +++ b/src/box/vy_upsert.c
> > @@ -102,15 +102,13 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
> >          const char *ups_ops = new_ops;
> >          /*
> >           * In case upsert folds into insert, we must skip first
> > -        * update operations. Moreover, we should create (in case of delete
> > -        * statement - re-create since delete contains only key) tuple with
> > -        * format to use it for PK modification check.
> > +        * update operations. Moreover, we should use upsert's tuple
> > +        * to provide PK modification check.
> >           */
> >          if (stmt_is_void) {
> >                  ups_cnt--;
> >                  mp_next(&ups_ops);
> > -               stmt = vy_stmt_new_replace(tuple_format(upsert), result_mp,
> > -                                          result_mp_end);
> > +               stmt = upsert;
> >          }
> >          for (uint32_t i = 0; i < ups_cnt; ++i) {
> >                  assert(mp_typeof(*ups_ops) == MP_ARRAY);
> > @@ -126,8 +124,6 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
> >                                  assert(e != NULL);
> >                                  /* Bail out immediately in case of OOM. */
> >                                  if (e->type != &type_ClientError) {
> > -                                       if (stmt_is_void)
> > -                                               tuple_unref(stmt);
> >                                          region_truncate(region, region_svp);
> >                                          return NULL;
> >                                  }
> > @@ -169,8 +165,6 @@ vy_apply_upsert_on_terminal_stmt(struct tuple *upsert, struct tuple *stmt,
> >          struct tuple *new_terminal_stmt = vy_stmt_new_replace(format, result_mp,
> >                                                                result_mp_end);
> >          region_truncate(region, region_svp);
> > -       if (stmt_is_void)
> > -               tuple_unref(stmt);
> >          if (new_terminal_stmt == NULL)
> >                  return NULL;
> >          vy_stmt_set_lsn(new_terminal_stmt, vy_stmt_lsn(upsert));
> >