Move the PredicateLockRelation() call from nodeSeqscan.c to heapam.c. It's

more consistent that way, since all the other PredicateLock* calls are
made in various heapam.c and index AM functions. The call in nodeSeqscan.c
was unnecessarily aggressive anyway, there's no need to try to lock the
relation every time a tuple is fetched, it's enough to do it once.

This has the user-visible effect that if a seq scan is initialized in the
executor, but never executed, we now acquire the predicate lock on the heap
relation anyway. We could avoid that by taking the lock on the first
heap_getnext() call instead, but it doesn't seem worth the trouble given
that it feels more natural to do it in heap_beginscan().

Also, remove the retail PredicateLockTuple() calls from heap_getnext(). In
a seqscan, started with heap_begin(), we're holding a whole-relation
predicate lock on the heap so there's no need to lock the tuples
individually.

Kevin Grittner and me

Author: hlinnaka

src/backend/access/heap/heapam.c

@@ -479,8 +479,6 @@ heapgettup(HeapScanDesc scan,
 
 				if (valid)
 				{
-					if (!scan->rs_relpredicatelocked)
-						PredicateLockTuple(scan->rs_rd, tuple, snapshot);
 					LockBuffer(scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
 					return;
 				}
@@ -748,16 +746,12 @@ heapgettup_pagemode(HeapScanDesc scan,
 							nkeys, key, valid);
 				if (valid)
 				{
-					if (!scan->rs_relpredicatelocked)
-						PredicateLockTuple(scan->rs_rd, tuple, scan->rs_snapshot);
 					scan->rs_cindex = lineindex;
 					return;
 				}
 			}
 			else
 			{
-				if (!scan->rs_relpredicatelocked)
-					PredicateLockTuple(scan->rs_rd, tuple, scan->rs_snapshot);
 				scan->rs_cindex = lineindex;
 				return;
 			}
@@ -1228,13 +1222,26 @@ heap_beginscan_internal(Relation relation, Snapshot snapshot,
 	scan->rs_strategy = NULL;	/* set in initscan */
 	scan->rs_allow_strat = allow_strat;
 	scan->rs_allow_sync = allow_sync;
-	scan->rs_relpredicatelocked = false;
 
 	/*
 	 * we can use page-at-a-time mode if it's an MVCC-safe snapshot
 	 */
 	scan->rs_pageatatime = IsMVCCSnapshot(snapshot);
 
+	/*
+	 * For a seqscan in a serializable transaction, acquire a predicate lock
+	 * on the entire relation. This is required not only to lock all the
+	 * matching tuples, but also to conflict with new insertions into the
+	 * table. In an indexscan, we take page locks on the index pages covering
+	 * the range specified in the scan qual, but in a heap scan there is
+	 * nothing more fine-grained to lock. A bitmap scan is a different story,
+	 * there we have already scanned the index and locked the index pages
+	 * covering the predicate. But in that case we still have to lock any
+	 * matching heap tuples.
+	 */
+	if (!is_bitmapscan)
+		PredicateLockRelation(relation, snapshot);
+
 	/* we only need to set this up once */
 	scan->rs_ctup.t_tableOid = RelationGetRelid(relation);
 

src/backend/executor/nodeSeqscan.c

@@ -28,7 +28,6 @@
 #include "access/relscan.h"
 #include "executor/execdebug.h"
 #include "executor/nodeSeqscan.h"
-#include "storage/predicate.h"
 
 static void InitScanRelation(SeqScanState *node, EState *estate);
 static TupleTableSlot *SeqNext(SeqScanState *node);
@@ -106,16 +105,11 @@ SeqRecheck(SeqScanState *node, TupleTableSlot *slot)
  *		tuple.
  *		We call the ExecScan() routine and pass it the appropriate
  *		access method functions.
- *		For serializable transactions, we first acquire a predicate
- *		lock on the entire relation.
  * ----------------------------------------------------------------
  */
 TupleTableSlot *
 ExecSeqScan(SeqScanState *node)
 {
-	PredicateLockRelation(node->ss_currentRelation,
-						  node->ss_currentScanDesc->rs_snapshot);
-	node->ss_currentScanDesc->rs_relpredicatelocked = true;
 	return ExecScan((ScanState *) node,
 					(ExecScanAccessMtd) SeqNext,
 					(ExecScanRecheckMtd) SeqRecheck);

src/include/access/relscan.h

@@ -35,7 +35,6 @@ typedef struct HeapScanDescData
 	BlockNumber rs_startblock;	/* block # to start at */
 	BufferAccessStrategy rs_strategy;	/* access strategy for reads */
 	bool		rs_syncscan;	/* report location to syncscan logic? */
-	bool		rs_relpredicatelocked;	/* predicate lock on relation exists */
 
 	/* scan current state */
 	bool		rs_inited;		/* false = scan not init'd yet */