On Mon, Mar 12, 2018 at 11:45 AM, amul sul <sula...@gmail.com> wrote: > 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.
I did the aforementioned changes in the attached patch, thanks. Regards, Amul
0001-Invalidate-ip_blkid-v6.patch
Description: Binary data
0002-isolation-tests-v6.patch
Description: Binary data
