From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id BA9E2469719 for ; Fri, 9 Oct 2020 18:06:58 +0300 (MSK) Date: Fri, 9 Oct 2020 15:06:57 +0000 From: Nikita Pettik Message-ID: <20201009150657.GA31117@tarantool.org> References: <743a25a986ebbe4388d8f6ffc7d1502f20a5efb9.1601729099.git.korablev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v3 1/2] vinyl: rework upsert operation List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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. > > */