From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (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 3B4EE469710 for ; Tue, 19 May 2020 21:18:25 +0300 (MSK) Date: Tue, 19 May 2020 18:18:24 +0000 From: Nikita Pettik Message-ID: <20200519181824.GA13813@tarantool.org> References: <670c3876e58a7cfa14d45db1dc074a10dd034759.1586808463.git.korablev@tarantool.org> <20200514022101.GC18509@tarantool.org> <5ed7e39d-35b7-e002-83e3-51f0ce2f898b@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5ed7e39d-35b7-e002-83e3-51f0ce2f898b@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 14 May 23:32, Vladislav Shpilevoy wrote: > Thanks for the fixes! > > On 14/05/2020 04:21, Nikita Pettik wrote: > > On 01 May 02:31, Vladislav Shpilevoy wrote: > >> Hi! Thanks for the patch! > >> > >> Firstly, Kostja left some comments here. Would be cool to address them. > > > > Done (sorry, I did not ignore them, just had to work on other more vital bugs). > > > >> Secondly, here is my personal opinion. I don't like just skipping things > >> a user committed without any error appearing in the application. IMO, we > >> should apply only the first commit. And let a user see this error so as he > >> could notice the problem. To fix reads he could do delete() of the bad key. > > > > The problem with delete it leaves user no way to restore the rest > > of upsert history. Moreover, these upserts will get stuck until > > user finds in logs corresponding error (I guess we can't abort > > compaction due to invalid upserts). > > > >> However, how a user will be able to find the exact broken key - I don't > >> know. Maybe the ignore + logging is better. > > > > Why can't we just log broken key? E.g. see logs in vy_apply_upsert(). > > We can log it. This is what you do in this patchset. > > I also noticed, that you skip the failed upsert in case of any error. > Even if it is an OOM, not related to format problems. I think it is safer > to check error type, and skip it only in case of a ClientError. Personally I don't like relying on error type, but this pattern is already in the source code (e.g. upsert_do_ops), so here's diff: diff --git a/src/box/vy_history.c b/src/box/vy_history.c index 8c4719a85..2232348f9 100644 --- a/src/box/vy_history.c +++ b/src/box/vy_history.c @@ -109,6 +109,9 @@ vy_history_apply(struct vy_history *history, struct key_def *cmp_def, * 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; } else { ++*upserts_applied; if (curr_stmt != NULL) diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c index aefad5942..1a2ea43d9 100644 --- a/src/box/vy_tx.c +++ b/src/box/vy_tx.c @@ -1050,6 +1050,8 @@ vy_tx_set(struct vy_tx *tx, struct vy_lsm *lsm, struct tuple *stmt) if (applied == NULL) { struct error *e = diag_last_error(diag_get()); assert(e != NULL); + if (e->type != &type_ClientError) + return -1; error_log(e); } else { lsm->stat.upsert.applied++; diff --git a/src/box/vy_write_iterator.c b/src/box/vy_write_iterator.c index 13d5bbede..4d34d96c8 100644 --- a/src/box/vy_write_iterator.c +++ b/src/box/vy_write_iterator.c @@ -857,6 +857,8 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint, */ 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); @@ -878,6 +880,8 @@ vy_read_view_merge(struct vy_write_iterator *stream, struct tuple *hint, 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 {