From 8a3e8e8bd038ae9ff2f7972b29cd8a4f66c93813 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Thu, 17 Oct 2024 11:45:36 -0400 Subject: [PATCH 04/12] 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. We need the hint bits to be set as otherwise reform_and_rewrite_tuple() -> rewrite_heap_tuple() -> heap_freeze_tuple() will get confused. It'd be better if we somehow coulda void setting hint bits on the old page. One reason to use VACUUM FULL are very bloated tables - rewriting most of the old table before during VACUUM FULL doesn't. Author: Reviewed-by: Discussion: https://postgr.es/m/ Backpatch: --- src/backend/access/heap/heapam_handler.c | 13 ++++++++++++- src/backend/access/heap/heapam_visibility.c | 7 +++++++ src/backend/storage/buffer/bufmgr.c | 2 +- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c index 24d3765aa20..d21d7d4cef7 100644 --- a/src/backend/access/heap/heapam_handler.c +++ b/src/backend/access/heap/heapam_handler.c @@ -837,7 +837,18 @@ 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 to be set + * as otherwise reform_and_rewrite_tuple() -> rewrite_heap_tuple() -> + * heap_freeze_tuple() will get confused. + * + * 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 before 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 4fefcbca5f5..71a410502b3 100644 --- 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(BufferIsLocal(buffer) || BufferLockHeldByMe(buffer, BUFFER_LOCK_EXCLUSIVE)); + SetHintBits(tuple, buffer, infomask, xid); } diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 8be8908c39c..5187f261423 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -5908,7 +5908,7 @@ BufferLockHeldByMe(Buffer buffer, int mode) { Assert(false); pg_unreachable(); - lwmode = LW_EXCLUSIVE; /* assuage compiler */ + lwmode = LW_EXCLUSIVE; /* assuage compiler */ } return LWLockHeldByMeInMode(BufferDescriptorGetContentLock(buf), lwmode); -- 2.39.5