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

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
> <pavan(dot)deolasee(at)gmail(dot)com> wrote:
>>
>> On Tue, Feb 13, 2018 at 12:41 PM, amul sul <sulamul(at)gmail(dot)com> wrote:
>>>
>>> Thanks for the confirmation, updated patch attached.
>>>
>>
>> I am actually very surprised that 0001-Invalidate-ip_blkid-v5.patch does not
>> do anything to deal with the fact that t_ctid may no longer point to itself
>> to mark end of the chain. I just can't see how that would work.
>>
>
> I think it is not that patch doesn't care about the end of the chain.
> For example, ctid pointing to itself is used to ensure that for
> deleted rows, nothing more needs to be done like below check in the
> ExecUpdate/ExecDelete code path.
> if (!ItemPointerEquals(tupleid, &hufd.ctid))
> {
> ..
> }
> ..
>
> It will deal with such cases by checking invalidblockid before these
> checks. So, we should be fine in such cases.
>
>
>> But if it
>> does, it needs good amount of comments explaining why and most likely
>> updating comments at other places where chain following is done. For
>> example, how's this code in heap_get_latest_tid() is still valid? Aren't we
>> setting "ctid" to some invalid value here?
>>
>> 2302 /*
>> 2303 * If there's a valid t_ctid link, follow it, else we're done.
>> 2304 */
>> 2305 if ((tp.t_data->t_infomask & HEAP_XMAX_INVALID) ||
>> 2306 HeapTupleHeaderIsOnlyLocked(tp.t_data) ||
>> 2307 ItemPointerEquals(&tp.t_self, &tp.t_data->t_ctid))
>> 2308 {
>> 2309 UnlockReleaseBuffer(buffer);
>> 2310 break;
>> 2311 }
>> 2312
>> 2313 ctid = tp.t_data->t_ctid;
>>
>
> I have not tested, but it seems this could be problematic, but I feel
> we could deal with such cases by checking invalid block id in the
> above if check. Another one such case is in EvalPlanQualFetch
>
>> 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.

Regards,
Amul

1] https://postgr.es/m/CAAJ_b96mw5xn5oSQgxpgn5dWFRs1j7OebpHRmXkdSNY+70yYEw@mail.gmail.com