From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 43EDD469710 for ; Thu, 14 May 2020 05:21:02 +0300 (MSK) Date: Thu, 14 May 2020 02:21:01 +0000 From: Nikita Pettik Message-ID: <20200514022101.GC18509@tarantool.org> References: <670c3876e58a7cfa14d45db1dc074a10dd034759.1586808463.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 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 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(). > On 13/04/2020 23:55, Nikita Pettik wrote: > > 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). > > > > Closes #1622 > > --- > > diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c > > index 5029bd8a1..060a7f6a9 100644 > > --- a/src/box/vy_tx.c > > +++ b/src/box/vy_tx.c > > @@ -515,11 +515,15 @@ vy_tx_write(struct vy_lsm *lsm, struct vy_mem *mem, > > region_stmt); > > tuple_unref(applied); > > return rc; > > + } else { > > + /* > > + * Ignore a memory error, because it is > > + * not critical to apply the optimization. > > + * Clear diag: otherwise error is set but > > + * function may return success return code. > > + */ > > + diag_clear(diag_get()); > > Why do you clear it? Diagnostics area is usually not cleared (at least > in application code), and contains some last happened error. In C code we > anyway use result value of a function to determine its result. Agree, forgot that we do not erase diag before each request execution. Removed this clean-up.