Double content-lock acquisition silently leaks a lock

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