From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 7F1B2469710 for ; Fri, 15 May 2020 00:32:39 +0300 (MSK) References: <670c3876e58a7cfa14d45db1dc074a10dd034759.1586808463.git.korablev@tarantool.org> <20200514022101.GC18509@tarantool.org> From: Vladislav Shpilevoy Message-ID: <5ed7e39d-35b7-e002-83e3-51f0ce2f898b@tarantool.org> Date: Thu, 14 May 2020 23:32:37 +0200 MIME-Version: 1.0 In-Reply-To: <20200514022101.GC18509@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 Cc: tarantool-patches@dev.tarantool.org 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.