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.

- 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.


if (mytup.t_data->t_infomask & HEAP_XMAX_INVALID ||
  ItemPointerEquals(&mytup.t_self, &mytup.t_data->t_ctid) ||
+ !HeapTupleHeaderValidBlockNumber(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

Reply via email to