On Thu, Mar 8, 2018 at 12:31 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > On Thu, Mar 8, 2018 at 11:04 AM, Pavan Deolasee > <pavan.deola...@gmail.com> wrote: >> >> On Tue, Feb 13, 2018 at 12:41 PM, amul sul <sula...@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. > 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+70y...@mail.gmail.com