From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42]) (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 DA7EF469710 for ; Mon, 1 Jun 2020 22:24:03 +0300 (MSK) Date: Mon, 1 Jun 2020 19:24:02 +0000 From: Nikita Pettik Message-ID: <20200601192402.GA9089@tarantool.org> References: <670c3876e58a7cfa14d45db1dc074a10dd034759.1586808463.git.korablev@tarantool.org> <30344cf8-3ea1-33c2-2c34-89a2c4c561ec@tarantool.org> <20200527200506.GC17325@tarantool.org> <7039aa7a-035a-8499-ff2c-de2b7509b788@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <7039aa7a-035a-8499-ff2c-de2b7509b788@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/2] vinyl: skip invalid upserts during squash 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 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 [20/05/14 09:50]: > On 14 Apr 01:12, Konstantin Osipov wrote: > > * Nikita Pettik [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.