Re: [Tarantool-patches] [PATCH 2/2] vinyl: skip invalid upserts during squash

[Vladislav Shpilevoy <v.shpilevoy@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.