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

[Aleksandr Lyapunov <alyapunov@tarantool.org>]

Great, LGTM!

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));
>
>