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

On Sat, Mar 10, 2018 at 5:25 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Mar 9, 2018 at 3:18 PM, amul sul <sulamul(at)gmail(dot)com> wrote:
>> On Thu, Mar 8, 2018 at 12:31 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

Thanks for the confirmation and suggestions.

Regards,
Amul