heapam: Use exclusive lock on old page in CLUSTER

To be able to guarantee that we can set the hint bit, acquire an exclusive
lock on the old buffer. This is required as a future commit will only allow
hint bits to be set with a new lock level, which is acquired as-needed in a
non-blocking fashion.

We need the hint bits, set in heapam_relation_copy_for_cluster() ->
HeapTupleSatisfiesVacuum(), to be set, as otherwise reform_and_rewrite_tuple()
-> rewrite_heap_tuple() will get confused. Specifically, rewrite_heap_tuple()
checks for HEAP_XMAX_INVALID in the old tuple to determine whether to check
the old-to-new mapping hash table.

It'd be better if we somehow could avoid setting hint bits on the old page. A
common reason to use VACUUM FULL is very bloated tables - rewriting most of
the old table during VACUUM FULL doesn't exactly help.

Reviewed-by: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Reviewed-by: Kirill Reshke <reshkekirill@gmail.com>
Discussion: https://postgr.es/m/4wggb7purufpto6x35fd2kwhasehnzfdy3zdcu47qryubs2hdz@fa5kannykekr

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 09a456e99661efa55b83e2ceff7184ab90c6331b..cbef73e5d4b1d33559855c9279dbd088f6ebb8c8 100644 (file)
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -837,7 +837,21 @@ heapam_relation_copy_for_cluster(Relation OldHeap, Relation NewHeap,
                tuple = ExecFetchSlotHeapTuple(slot, false, NULL);
                buf = hslot->buffer;
 
-               LockBuffer(buf, BUFFER_LOCK_SHARE);
+               /*
+                * To be able to guarantee that we can set the hint bit, acquire an
+                * exclusive lock on the old buffer. We need the hint bits, set in
+                * heapam_relation_copy_for_cluster() -> HeapTupleSatisfiesVacuum(),
+                * to be set, as otherwise reform_and_rewrite_tuple() ->
+                * rewrite_heap_tuple() will get confused. Specifically,
+                * rewrite_heap_tuple() checks for HEAP_XMAX_INVALID in the old tuple
+                * to determine whether to check the old-to-new mapping hash table.
+                *
+                * It'd be better if we somehow could avoid setting hint bits on the
+                * old page. One reason to use VACUUM FULL are very bloated tables -
+                * rewriting most of the old table during VACUUM FULL doesn't exactly
+                * help...
+                */
+               LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
 
                switch (HeapTupleSatisfiesVacuum(tuple, OldestXmin, buf))
                {

diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index 05e70b7d92ae3a95ac43c0537709c552b280c9c5..9a034d5c9e8e47058b78253b91b96e90a0540e5f 100644 (file)
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -141,6 +141,13 @@ void
 HeapTupleSetHintBits(HeapTupleHeader tuple, Buffer buffer,
                                         uint16 infomask, TransactionId xid)
 {
+       /*
+        * The uses from heapam.c rely on being able to perform the hint bit
+        * updates, which can only be guaranteed if we are holding an exclusive
+        * lock on the buffer - which all callers are doing.
+        */
+       Assert(BufferIsLockedByMeInMode(buffer, BUFFER_LOCK_EXCLUSIVE));
+
        SetHintBits(tuple, buffer, infomask, xid);
 }
 

diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index bae3a2da77a0fc3bdf1a09a37b7339b943295706..77fd48eb59ee9138eb689bbbdb0767fef695d9c7 100644 (file)
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -382,6 +382,9 @@ rewrite_heap_tuple(RewriteState state,
 
        /*
         * If the tuple has been updated, check the old-to-new mapping hash table.
+        *
+        * Note that this check relies on the HeapTupleSatisfiesVacuum() in
+        * heapam_relation_copy_for_cluster() to have set hint bits.
         */
        if (!((old_tuple->t_data->t_infomask & HEAP_XMAX_INVALID) ||
                  HeapTupleHeaderIsOnlyLocked(old_tuple->t_data)) &&