On Thu, Mar 8, 2018 at 3:01 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Mar 8, 2018 at 12:52 PM, Pavan Deolasee
> <pavan.deolasee@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
>>> <pavan.deolasee@gmail.com> wrote:
>>> >
>>> > On Tue, Feb 13, 2018 at 12:41 PM, amul sul <sulamul@gmail.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.
>>
>>
>> Yeah, but it only looks for places where it needs to detect deleted tuples
>> and thus wants to throw an error. I am worried about other places where it
>> is assumed that the ctid points to a valid looking tid, self or otherwise. I
>> see no such places being either updated or commented.
>>
>> Now may be there is no danger because of other protections in place, but it
>> looks hazardous.
>>
>
> Right, I feel we need some tests to prove it, I think as per code I
> can see we need checks in few more places (like the ones mentioned in
> the previous email) apart from where this patch has added.
>
>>>
>>>
>>> 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
>>>
>>
>> Right.
>>
>
> Amul, can you please look into the scenario being discussed and see if
> you can write a test to see the behavior.
>
Sure, I'll try.
Regards,
Amu
