From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (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 36CF74696C3 for ; Fri, 1 May 2020 03:31:04 +0300 (MSK) References: <670c3876e58a7cfa14d45db1dc074a10dd034759.1586808463.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 1 May 2020 02:31:02 +0200 MIME-Version: 1.0 In-Reply-To: <670c3876e58a7cfa14d45db1dc074a10dd034759.1586808463.git.korablev@tarantool.org> 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! Firstly, Kostja left some comments here. Would be cool to address them. 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. However, how a user will be able to find the exact broken key - I don't know. Maybe the ignore + logging is better. 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.