From 29781b25cdad9295a0a87ac23213d694725c89d0 Mon Sep 17 00:00:00 2001 From: Greg Burd 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