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

On Thu, May 17, 2018 at 3:49 AM Heikki Linnakangas <hlinnaka(at)iki(dot)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