On Fri, Apr 6, 2018 at 12:07 PM, amul sul <sulamul@gmail.com> wrote:
> On Thu, Apr 5, 2018 at 10:17 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Thu, Apr 5, 2018 at 7:14 AM, Andres Freund <andres@anarazel.de> wrote:
>>>
> [...]
>>>
>>> Questions:
>>>
>>> - I'm not perfectly happy with
>>> "tuple to be locked was already moved to another partition due to concurrent update"
>>> as the error message. If somebody has a better suggestions.
>>>
>>
>> I don't have any better suggestion, but I have noticed a small
>> inconsistency in the message. In case of delete, the message is
>> "tuple to be updated was ...". I think here it should be "tuple to be
>> deleted was ...".
>>
>
> +1, will do the error message change in ExecDelete.
>
>>> - should heap_get_latest_tid() error out when the chain ends in a moved
>>> tuple?
>>
>> Won't the same question applies to the similar usage in
>> EvalPlanQualFetch and heap_lock_updated_tuple_rec. In
>> EvalPlanQualFetch, we consider such a tuple to be deleted and will
>> silently miss/skip it which seems contradictory to the places where we
>> have detected such a situation and raised an error. In
>> heap_lock_updated_tuple_rec, we will skip locking the versions of a
>> tuple after we encounter a tuple version that is moved to another
>> partition.
>>
>>> - I'm not that happy with the number of added spec test files with
>>> number postfixes. Can't we combine them into a single file?
>>
>> +1 for doing so.
>
> Agree, we could combine specs-1/2/3 into a single file which is doing the error
> check and for the specs-4/5, imho, let it be, as it is checking different the
> scenario of ON CONFLICT DO NOTHING on the moved tuple and also
> it resembles the existing ON CONFLICT isolation tests.
>
> Will post rebase version of Andres' patch[1] including aforementioned
> changes within an hour, thanks
>
>
> 1] https://postgr.es/m/20180405014439.fbezvbjrmcw64vjc@alap3.anarazel.de
>
Updated patch attached.
Regards,
Amul