From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 AE52B469711 for ; Wed, 27 May 2020 00:33:57 +0300 (MSK) From: Vladislav Shpilevoy References: <670c3876e58a7cfa14d45db1dc074a10dd034759.1586808463.git.korablev@tarantool.org> Message-ID: <30344cf8-3ea1-33c2-2c34-89a2c4c561ec@tarantool.org> Date: Tue, 26 May 2020 23:33:55 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Nikita Pettik , tarantool-patches@dev.tarantool.org 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? > 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();. > 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. > 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? > @@ -864,10 +877,17 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint, > assert(result->tuple != NULL); > struct tuple *applied = vy_apply_upsert(h->tuple, result->tuple, > stream->cmp_def, stream->format, false); > - if (applied == NULL) > - return -1; > - vy_stmt_unref_if_possible(result->tuple); > - result->tuple = applied; > + if (applied == NULL) { > + 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); > + } else { > + vy_stmt_unref_if_possible(result->tuple); > + result->tuple = applied; > + } > vy_stmt_unref_if_possible(h->tuple); > /* > * Don't bother freeing 'h' since it's