From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [94.100.177.89]) (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 9F58A469710 for ; Thu, 21 May 2020 01:13:08 +0300 (MSK) References: <670c3876e58a7cfa14d45db1dc074a10dd034759.1586808463.git.korablev@tarantool.org> <20200514022101.GC18509@tarantool.org> <5ed7e39d-35b7-e002-83e3-51f0ce2f898b@tarantool.org> <20200519181824.GA13813@tarantool.org> From: Vladislav Shpilevoy Message-ID: <6ab06c70-59fe-57c0-a989-7c52574f89bb@tarantool.org> Date: Thu, 21 May 2020 00:13:06 +0200 MIME-Version: 1.0 In-Reply-To: <20200519181824.GA13813@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. > > 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: Yeah, I agree. Error type is rather a crutch. And should be used only in userspace code. As an alternative, you can add an out parameter to vy_apply_upsert(), 'is_format_invalid' or something like this, and set it to true in case of tuple_validate_raw() error. Set to false for any other errors. However error type solution also looks ok, so up to you.