Re: [HACKERS] Restrict concurrent update/delete with UPDATE ofpartition key

On Mon, Mar 12, 2018 at 11:45 AM, amul sul <sulamul@gmail.com> wrote:
> On Sat, Mar 10, 2018 at 5:25 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Fri, Mar 9, 2018 at 3:18 PM, amul sul <sulamul@gmail.com> wrote:
>>> On Thu, Mar 8, 2018 at 12:31 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>>> On Thu, Mar 8, 2018 at 11:04 AM, Pavan Deolasee
>>>>
>>>>> This is just one example. I am almost certain there are many such cases that
>>>>> will require careful attention.
>>>>>
>>>>
>>>> Right, I think we should be able to detect and fix such cases.
>>>>
>>>
>>> I found a couple of places (in heap_lock_updated_tuple, rewrite_heap_tuple,
>>> EvalPlanQualFetch & heap_lock_updated_tuple_rec) where ItemPointerEquals is
>>> use to check tuple has been updated/deleted.  With the proposed patch
>>> ItemPointerEquals() will no longer work as before, we required addition check
>>> for updated/deleted tuple, proposed the same in latest patch[1]. Do let me know
>>> your thoughts/suggestions on this, thanks.
>>>
>>
>> I think you have identified the places correctly.  I have few
>> suggestions though.
>>
>> 1.
>> - if (!ItemPointerEquals(&tuple->t_self, ctid))
>> + if (!(ItemPointerEquals(&tuple->t_self, ctid) ||
>> +   (!ItemPointerValidBlockNumber(ctid) &&
>> +    (ItemPointerGetOffsetNumber(&tuple->t_self) ==   /* TODO: Condn.
>> should be macro? */
>> + ItemPointerGetOffsetNumber(ctid)))))
>>
>> Can't we write this and similar tests as:
>> ItemPointerValidBlockNumber(ctid) &&
>> !ItemPointerEquals(&tuple->t_self, ctid)?  It will be bit simpler to
>> understand and serve the purpose.
>>
>
> Yes, you are correct, we need not worry about offset matching -- invalid block
> number check and ItemPointerEquals is more than enough to conclude the tuple has
> been deleted or not.  Will change the condition accordingly in the next version.
>
>> 2.
>>
>> if (mytup.t_data->t_infomask & HEAP_XMAX_INVALID ||
>>   ItemPointerEquals(&mytup.t_self, &mytup.t_data->t_ctid) ||
>> + !HeapTupleHeaderValidBlockNumber(mytup.t_data) ||
>>   HeapTupleHeaderIsOnlyLocked(mytup.t_data))
>>
>> I think it is better to keep the check for
>> HeapTupleHeaderValidBlockNumber earlier than ItemPointerEquals as it
>> will first validate if block number is valid and then only compare the
>> complete CTID.
>
> Sure, will do that.

I did the aforementioned changes in the attached patch, thanks.

Regards,
Amul