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. 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. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
