Re: Double content-lock acquisition silently leaks a lock

Hi,

On 2026-09-22 14:54:33 +0000, Greg Burd wrote:
> 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.

That's completely normal for assertion paths IME.

> Patch attached to turn the assertion into an error. I am not proposing it
> as the whole fix; details and an open question below.

-0.5, it shows up in profiles for me. If we had support for something like
production assertions, with a lower overhead, I'd maybe +0.5 instead, but we
don't.

And I am fundamentally opposed to not make this an assertion failure. This is
a fundamentally broken program, that needs to trigger assertion
failures. There's way way too many contexts where an ERROR is basically
invisible.

> 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:

There's no assymmetry, acquiring the same lock twice is fundamentally
wrong. It isn't supported. It was never supported. There's an assertion about
it.

> Also, error recovery does not rescue it.

Obviously not.

Greetings,

Andres Freund