Thread: Re: Repeated PredicateLockRelation calls during seqscan

Heikki Linnakangas  wrote:

> BTW, isn't bitgetpage() in nodeBitmapHeapscan.c missing
> PredicateLockTuple() and CheckForSerializableConflictOut() calls in
> the codepath for a lossy bitmap? In the non-lossy case,
> heap_hot_search_buffer() takes care of it, but not in the lossy
> case.

I think the attached addresses that.

In looking this over I noticed something else that doesn't seem quite
right.  In heapam.c there are two places where the execution of
PredicateLockTuple() is conditioned not just on MVCC visibility, but
also on HeapKeyTest().  I think those calls should be moved to not be
conditioned on that.  Otherwise we have a predicate condition being
tested without "locking the gaps", don't we?

-Kevin

On 25.06.2011 22:29, Kevin Grittner wrote:
> In looking this over I noticed something else that doesn't seem quite
> right.  In heapam.c there are two places where the execution of
> PredicateLockTuple() is conditioned not just on MVCC visibility, but
> also on HeapKeyTest().  I think those calls should be moved to not be
> conditioned on that.  Otherwise we have a predicate condition being
> tested without "locking the gaps", don't we?

Locks on heap tuples don't serve the purpose of locking gaps, anyway, 
because you can't lock anything that doesn't yet exist that way. Locks 
on index pages and the heap relation serve that purpose.

I wonder if we need those PredicateLockTuple() calls in heapam.c at all. 
You need to take a whole-relation lock on the heap to lock the gaps, to 
ensure that you conflict newly inserted rows. And if fetch a tuple via 
an index, you acquire locks on the index pages. What is the point of the 
PredicateLockTuple() calls? We added the rs_relpredicatelocked mechanism 
as an optimization, but I'm thinking that it's actually required for 
correctness to grab a predicate lock on the whole relation when you do a 
seqscan.

--   Heikki Linnakangas  EnterpriseDB   http://www.enterprisedb.com