On Sat, Mar 10, 2018 at 5:25 PM, Amit Kapila <amit.kapil...@gmail.com> wrote: > On Fri, Mar 9, 2018 at 3:18 PM, amul sul <sula...@gmail.com> wrote: >> 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 >>> >>>> 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
