Hi hackers,
BufferLockAcquire() only asserts that the current backend does not already hold
a content lock on the buffer. I hit that assertion on a buildfarm animal, and
while chasing it I convinced myself that the non-assert behaviour is worse than
the crash in that it can permanently wedges the buffer. Patch attached to turn
the assertion into an error. I am not proposing it as the whole fix; details
and an open question below.
There is a bookkeeping asymmetry, PrivateRefCountData has room for a single
lockmode...
typedef struct PrivateRefCountData
{
int32 refcount; /* per-pin */
BufferLockMode lockmode; /* per-buffer, ONE slot */
} PrivateRefCountData;
... but the shared lock state is counted per acquisition:
BufferLockAttempt(): desired_state += BM_LOCK_VAL_SHARED
BufferLockAcquire(): entry->data.lockmode = mode
BufferLockDisownInternal(): mode = ref->data.lockmode; ... = UNLOCK
BufferLockUnlock(): pg_atomic_sub_fetch_u64(&buf_hdr->state,
BufferLockReleaseSub(mode))
So am I wrong to say that a second share acquisition adds a second
BM_LOCK_VAL_SHARED to buf_hdr->state, while the assignment of
entry->data.lockmode overwrites the record of the first?
Assuming I'm correct, the release path then subtracts one but, that means that
the buffer is left permanently one shared locker too high. I see nothing that
will ever take that shared locker away, so the buffer can never again be locked
exclusively or share-exclusively. Or am I missing something...
In practice that is a VACUUM (or any exclusive waiter) blocked on that buffer
for the remaining life of the cluster.
Also, error recovery does not rescue it. ResOwnerReleaseBuffer() is the
mechanism fcb9c977aa5 relies on instead of LWLockReleaseAll(), and it also
releases at most one lock per buffer:
if (ref->data.lockmode != BUFFER_LOCK_UNLOCK)
BufferLockUnlock(buffer, buf);
So with assertions enabled we crash, and with assertions disabled we silently
leak a content lock. I would much rather have the crash.
This is new in 19, this situation used to be representable. When content locks
were lwlocks, held locks were tracked in an array...
static LWLockHandle held_lwlocks[MAX_SIMUL_LWLOCKS];
held_lwlocks[num_held_lwlocks].lock = lock;
held_lwlocks[num_held_lwlocks++].mode = mode;
... so acquiring a share lock twice on the same buffer was fine, and
LWLockReleaseAll() released both entries on error.
fcb9c977aa5 replaced that with the single lockmode field. 333f586372a then
made the *conditional* path fail cleanly when the buffer is already locked by
this backend, with the reasoning that "we currently do not have space to track
multiple lock acquisitions on a single buffer". The unconditional path kept
only the assertion, and that is the gap this patch closes.
The patch will promote the assertion to an unconditional error, as in:
if (unlikely(entry->data.lockmode != BUFFER_LOCK_UNLOCK))
elog(ERROR, "buffer %d is already locked by this backend", buffer);
It sits before HOLD_INTERRUPTS(), so it does not disturb the interrupt state,
and there is precedent for elog(ERROR) in this code (BufferLockDisownInternal()
does "lock %d is not held"). An error is contained by normal error handling,
which releases the single lock that is recorded; a leaked lock is not
containable at all.
To be clear about scope, this does NOT fix whatever caller double-acquires. It
converts an unrecoverable silent wedge into a contained, diagnosable failure.
If a caller does this on a hot path, an error is still bad, but it is a bug we
can find, and today it is a bug we cannot even see outside assert builds.
Built with --enable-cassert and exercised the paths I could: create/index,
VACUUM, VACUUM FULL, VACUUM FREEZE, CLUSTER, REINDEX, ANALYZE of a user table
and of pg_class (the latter being what my animal was doing when it tripped). No
occurrence of the new error, no assertion failures, no unexpected errors, so
the guard does not misfire on ordinary work. I was not able to run the full
regression suite in my sandbox (pg_regress fails there with `could not exec
"sh"`, identically on an unpatched tree, so that is environmental rather than
caused by the change).
The assertion fired on my buildfarm animal "unicorn" which is aarch64 Windows
11 + MSVC. I compile with cassert on, injection_points on, in an autovacuum
worker running ANALYZE pg_catalog.pg_class, with BufferLockAcquire inlined into
LockBufferInternal and reached from heap_prepare_pagescan(). From the minidump
(rebuilt at the crashing commit for matching symbols, PrivateRefCountArray
decoded directly):
PrivateRefCountOverflowed = 0
slot 6: buffer=184 refcount=2 lockmode=1 (BUFFER_LOCK_SHARE)
slot 7: buffer=185 refcount=2 lockmode=0 (BUFFER_LOCK_UNLOCK)
and the buffer being locked at the fault was 184. Note refcount=2: the buffer
was pinned twice. That is consistent with two independent pin+lock sites in one
backend meeting on the same buffer, which the refcount can represent and the
lockmode cannot.
Question, I could not find which caller takes the second lock, but I suspect
that heapam_scan_analyze_next_block() takes a share lock and deliberately holds
it across the page ("we also choose to hold sharelock on the buffer
throughout"), releasing it only in heapam_scan_analyze_next_tuple(); if
anything in that window reaches a pagemode heap scan on the same buffer,
heap_prepare_pagescan() takes a second share lock. Both
heap_prepare_pagescan() and heap_page_prune_opt() are internally balanced, so
the pre-existing lock was held on entry from further up. I could not prove the
chain because the outer frames are optimised out, and I did not want to guess
in a commit message.
I have a machine that reproduces the assertion and am happy to run a build with
extra instrumentation, for example recording the acquiring stack in the
refcount entry, if that would help identify the caller.
best.
-greg
From 29781b25cdad9295a0a87ac23213d694725c89d0 Mon Sep 17 00:00:00 2001
From: Greg Burd <[email protected]>
Date: Mon, 21 Sep 2026 11:26:31 -0400
Subject: [PATCH] bufmgr: Turn double content-lock acquisition into an error
BufferLockAcquire() only asserted that the current backend does not already
hold a content lock on the buffer. In a non-assert build a second acquisition
proceeds, and the consequences are worse than a failed assertion.
PrivateRefCountData has room for a single lockmode, so only one lock
acquisition per buffer per backend can be tracked. A second acquisition adds
another lock-value to buf_hdr->state in BufferLockAttempt(), but the
assignment of entry->data.lockmode overwrites the record of the lock that was
already held. The eventual release subtracts only one lock-value, so the
buffer's lock state is left permanently too high and the buffer can never
again be locked exclusively: any later exclusive or share-exclusive waiter,
such as VACUUM, waits forever. Error recovery does not help, because
ResOwnerReleaseBuffer() also releases at most one lock per buffer.
This was previously representable: when content locks were lwlocks, held
locks were tracked in an array (held_lwlocks[]) and LWLockReleaseAll()
released each entry, so a backend taking a share lock twice on the same
buffer worked and recovered correctly. fcb9c977aa5 replaced that array with
the single lockmode field, and 333f586372a made the conditional path fail
cleanly in this situation, noting that there is no space to track multiple
lock acquisitions. The unconditional path was left with only the assertion.
Promote it to an unconditional elog(ERROR). Leaking a content lock is
unrecoverable and hard to diagnose, whereas an error is contained by the
normal error handling, which releases the one lock that is recorded. This
does not fix any caller that double-acquires; it makes such a bug fail
loudly in production instead of silently wedging a buffer.
Discussion: https://postgr.es/m/
---
src/backend/storage/buffer/bufmgr.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c
b/src/backend/storage/buffer/bufmgr.c
index 5c82865a084..14f0f7c5bf4 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -5930,9 +5930,26 @@ BufferLockAcquire(Buffer buffer, BufferDesc *buf_hdr,
BufferLockMode mode)
entry = GetPrivateRefCountEntry(buffer, true);
/*
- * We better not already hold a lock on the buffer.
+ * We must not already hold a lock on this buffer. Only one lock
+ * acquisition per buffer per backend can be tracked, as
+ * PrivateRefCountData has room for a single lockmode; see also the
+ * comment in BufferLockConditional(), which fails rather than acquire a
+ * second lock.
+ *
+ * Were we to proceed, BufferLockAttempt() below would add another
+ * lock-value to buf_hdr->state, but the assignment of
+ * entry->data.lockmode further down would overwrite the record of the
+ * lock we already hold. The eventual release would then subtract only
+ * one lock-value, permanently leaving the buffer's lock state too high,
+ * so that the buffer could never again be locked exclusively. Error
+ * recovery would not save us either, as ResOwnerReleaseBuffer()
likewise
+ * releases at most one lock per buffer.
+ *
+ * Leaking a content lock that way is unrecoverable and hard to
diagnose,
+ * so refuse to do it even in non-assert builds.
*/
- Assert(entry->data.lockmode == BUFFER_LOCK_UNLOCK);
+ if (unlikely(entry->data.lockmode != BUFFER_LOCK_UNLOCK))
+ elog(ERROR, "buffer %d is already locked by this backend",
buffer);
/*
* Lock out cancel/die interrupts until we exit the code section
protected
--
2.54.0
