From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 768BB469710 for ; Wed, 27 May 2020 23:05:07 +0300 (MSK) Date: Wed, 27 May 2020 20:05:06 +0000 From: Nikita Pettik Message-ID: <20200527200506.GC17325@tarantool.org> References: <670c3876e58a7cfa14d45db1dc074a10dd034759.1586808463.git.korablev@tarantool.org> <30344cf8-3ea1-33c2-2c34-89a2c4c561ec@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <30344cf8-3ea1-33c2-2c34-89a2c4c561ec@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 26 May 23:33, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > > diff --git a/src/box/vy_history.c b/src/box/vy_history.c > > index 0f3b71195..2232348f9 100644 > > --- a/src/box/vy_history.c > > +++ b/src/box/vy_history.c > > @@ -102,12 +102,22 @@ vy_history_apply(struct vy_history *history, struct key_def *cmp_def, > > while (node != NULL) { > > struct tuple *stmt = vy_apply_upsert(node->stmt, curr_stmt, > > cmp_def, format, true); > > - ++*upserts_applied; > > - if (curr_stmt != NULL) > > - tuple_unref(curr_stmt); > > - if (stmt == NULL) > > - return -1; > > - curr_stmt = stmt; > > + if (stmt == NULL) { > > + /* > > + * In case statement hasn't been applied, > > + * simply skip it ignoring errors (otherwise > > + * error will appear during tuple read). > > + */ > > + assert(diag_last_error(diag_get()) != NULL); > > + struct error *e = diag_last_error(diag_get()); > > + if (e->type != &type_ClientError) > > + return -1; > > *. Shouldn't we log here? No, we shouldn't. vy_history_apply() is called only during reads. So in order to avoid spam the same error each time on data selection, we don't log it here. > > diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c > > index 04c9926a8..3d630fc01 100644 > > --- a/src/box/vy_lsm.c > > +++ b/src/box/vy_lsm.c > > @@ -984,13 +984,20 @@ vy_lsm_commit_upsert(struct vy_lsm *lsm, struct vy_mem *mem, > > struct tuple *upserted = > > vy_apply_upsert(stmt, older, lsm->cmp_def, > > lsm->mem_format, false); > > - lsm->stat.upsert.applied++; > > - > > if (upserted == NULL) { > > - /* OOM */ > > + /* > > + * OOM or upserted tuple does not fulfill > > + * space format. Hence, upsert can't be > > + * transformed into replace. Log error > > + * and exit. > > + */ > > + struct error *e = diag_last_error(diag_get()); > > + assert(e != NULL); > > + error_log(e); > > *. These three lines can be replaced with diag_log();. Ok, let's use diag_log(): diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c index 3d630fc01..c768d0229 100644 --- a/src/box/vy_lsm.c +++ b/src/box/vy_lsm.c @@ -991,10 +991,7 @@ vy_lsm_commit_upsert(struct vy_lsm *lsm, struct vy_mem *mem, * transformed into replace. Log error * and exit. */ - struct error *e = diag_last_error(diag_get()); - assert(e != NULL); - error_log(e); - diag_clear(diag_get()); + diag_log(); return; } lsm->stat.upsert.applied++; diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c index 1a2ea43d9..0a14478fc 100644 --- a/src/box/vy_tx.c +++ b/src/box/vy_tx.c @@ -516,9 +516,7 @@ vy_tx_write(struct vy_lsm *lsm, struct vy_mem *mem, tuple_unref(applied); return rc; } else { - struct error *e = diag_last_error(diag_get()); - assert(e != NULL); - error_log(e); + diag_log(); } } } else { > > diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c > > index 5029bd8a1..1a2ea43d9 100644 > > --- a/src/box/vy_tx.c > > +++ b/src/box/vy_tx.c > > @@ -515,11 +515,11 @@ vy_tx_write(struct vy_lsm *lsm, struct vy_mem *mem, > > region_stmt); > > tuple_unref(applied); > > return rc; > > + } else { > > + struct error *e = diag_last_error(diag_get()); > > + assert(e != NULL); > > + error_log(e); > > *. The same here. See diff above. > > 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(): diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c index 4d34d96c8..ce700f260 100644 --- a/src/box/vy_write_iterator.c +++ b/src/box/vy_write_iterator.c @@ -859,8 +859,8 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint, 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); + say_error("upsert %s is not applied due to the error: %s", + vy_stmt_str(h->tuple), e->errmsg); vy_stmt_unref_if_possible(h->tuple); h->tuple = hint; } else { @@ -882,8 +882,8 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint, 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); + say_error("upsert %s is not applied due to the error: %s", + vy_stmt_str(h->tuple), e->errmsg); } else { vy_stmt_unref_if_possible(result->tuple); result->tuple = applied;