sample scans and predicate locking

Hi,

While looking at fixing [1] on master, I noticed the following
codeblock:

static HeapScanDesc
heap_beginscan_internal(Relation relation, Snapshot snapshot,
                        int nkeys, ScanKey key,
                        ParallelHeapScanDesc parallel_scan,
                        bool allow_strat,
                        bool allow_sync,
                        bool allow_pagemode,
                        bool is_bitmapscan,
                        bool is_samplescan,
                        bool temp_snap)
...
    /*
     * 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);

As you can see this only tests for is_bitmapscan, *not* for
is_samplescan. Which means we afaict currently every sample scan
predicate locks the entire relation.

I think there's two possibilities here:

1) It's just the comment that's inaccurate, and it should really talk
   about both seqscans and sample scans. It should not be necessary to
   lock the whole relation, but I'm not sure the code otherwise takes
   enough care.

2) We should really not predicate lock the entire relation. In which
   case I think there might be missing PredicateLockTuple/Page calls.

Greetings,

Andres Freund

[1] https://postgr.es/m/4EA80A20-E9BF-49F1-9F01-5B66CAB21453%40elusive.cx