Re: An out-of-date comment in nodeIndexonlyscan.c

On Thu, May 17, 2018 at 3:49 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 14/05/18 02:15, Thomas Munro wrote:
> > Since commit cdf91edb (2012), nodeIndexonlyscan.c says:
> >
> >                  /*
> >                   * Predicate locks for index-only scans must be
> > acquired at the page
> >                   * level when the heap is not accessed, since
> > tuple-level predicate
> >                   * locks need the tuple's xmin value.  If we had to
> > visit the tuple
> >                   * anyway, then we already have the tuple-level lock
> > and can skip the
> >                   * page lock.
> >                   */
> >                  if (tuple == NULL)
> >                          PredicateLockPage(scandesc->heapRelation,
> >
> > ItemPointerGetBlockNumber(tid),
> >                                                            estate->es_snapshot);
> >
> > The first sentence of that comment is no longer true as of commit
> > c01262a8 (2013).  As for whether it's necessary to predicate-lock the
> > whole eheap page (rather than the heap tuple) anyway because of HOT
> > update chains, I don't know, so I'm not sure what wording to propose
> > instead.
>
> Hmm. If there are any updated tuples, HOT or not, the visibility map bit
> will not be set, and we won't reach this code. So I think we could
> acquire the tuple lock here.

Right.  CC Kevin.  I think we should at least fix the comment.  As for
changing the lock level, PredicateLockTuple() wants a heap tuple that
we don't have, so we'd probably need to add a PredicateLockTid()
function.

-- 
Thomas Munro
http://www.enterprisedb.com