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
Regards,
Amul