Date: Tue, 22 Sep 2026 14:54:33 +0000
To: pgsql-hackers@lists.postgresql.org
From: Greg Burd <greg@burd.me>
Subject: Double content-lock acquisition silently leaks a lock
Message-ID: <arKVu9wp5A7EdKkx@floki>
Feedback-ID: 2668248:user:proton
MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="b1=_qB0dbzjOgWqZEcVR4yfDCr6wBhsUaLVrEhPjPybuQ"
Archived-At: <https://www.postgresql.org/message-id/arKVu9wp5A7EdKkx%40floki>
Precedence: bulk

--b1=_qB0dbzjOgWqZEcVR4yfDCr6wBhsUaLVrEhPjPybuQ
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

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; detai=
ls
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 +=3D BM_LOCK_VAL_SHARED
     BufferLockAcquire():          entry->data.lockmode =3D mode
     BufferLockDisownInternal():   mode =3D ref->data.lockmode; ... =3D UNL=
OCK
     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 t=
hat
the buffer is left permanently one shared locker too high. I see nothing th=
at
will ever take that shared locker away, so the buffer can never again be lo=
cked
exclusively or share-exclusively. Or am I missing something...

In practice that is a VACUUM (or any exclusive waiter) blocked on that buff=
er
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 !=3D BUFFER_LOCK_UNLOCK)
             BufferLockUnlock(buffer, buf);

So with assertions enabled we crash, and with assertions disabled we silent=
ly
leak a content lock.  I would much rather have the crash.

This is new in 19, this situation used to be representable.  When content l=
ocks
were lwlocks, held locks were tracked in an array...

     static LWLockHandle held_lwlocks[MAX_SIMUL_LWLOCKS];
     held_lwlocks[num_held_lwlocks].lock =3D lock;
     held_lwlocks[num_held_lwlocks++].mode =3D 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 tr=
ack
multiple lock acquisitions on a single buffer".  The unconditional path kep=
t
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 !=3D 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 stat=
e,
and there is precedent for elog(ERROR) in this code (BufferLockDisownIntern=
al()
does "lock %d is not held").  An error is contained by normal error handlin=
g,
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 failur=
e.
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 tab=
le
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, s=
o
the guard does not misfire on ordinary work.  I was not able to run the ful=
l
regression suite in my sandbox (pg_regress fails there with `could not exec
"sh"`, identically on an unpatched tree, so that is environmental rather th=
an
caused by the change).

The assertion fired on my buildfarm animal "unicorn" which is aarch64 Windo=
ws
11 + MSVC.  I compile with cassert on, injection_points on, in an autovacuu=
m
worker running ANALYZE pg_catalog.pg_class, with BufferLockAcquire inlined =
into
LockBufferInternal and reached from heap_prepare_pagescan().  From the mini=
dump
(rebuilt at the crashing commit for matching symbols, PrivateRefCountArray
decoded directly):

     PrivateRefCountOverflowed =3D 0
     slot 6: buffer=3D184  refcount=3D2  lockmode=3D1 (BUFFER_LOCK_SHARE)
     slot 7: buffer=3D185  refcount=3D2  lockmode=3D0 (BUFFER_LOCK_UNLOCK)

and the buffer being locked at the fault was 184.  Note refcount=3D2: the b=
uffer
was pinned twice.  That is consistent with two independent pin+lock sites i=
n one
backend meeting on the same buffer, which the refcount can represent and th=
e
lockmode cannot.

Question, I could not find which caller takes the second lock, but I suspec=
t
that heapam_scan_analyze_next_block() takes a share lock and deliberately h=
olds
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 gue=
ss
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

--b1=_qB0dbzjOgWqZEcVR4yfDCr6wBhsUaLVrEhPjPybuQ
Content-Type: text/plain; charset=us-ascii;
 name=v1-0001-bufmgr-double-content-lock-error.patch
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename=v1-0001-bufmgr-double-content-lock-error.patch

RnJvbSAyOTc4MWIyNWNkYWQ5Mjk1YTBhODdhYzIzMjEzZDY5NDcyNWM4OWQwIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQ0KRnJvbTogR3JlZyBCdXJkIDxncmVnQGJ1cmQubWU+DQpEYXRlOiBNb24s
IDIxIFNlcCAyMDI2IDExOjI2OjMxIC0wNDAwDQpTdWJqZWN0OiBbUEFUQ0hdIGJ1Zm1ncjogVHVy
biBkb3VibGUgY29udGVudC1sb2NrIGFjcXVpc2l0aW9uIGludG8gYW4gZXJyb3INCg0KQnVmZmVy
TG9ja0FjcXVpcmUoKSBvbmx5IGFzc2VydGVkIHRoYXQgdGhlIGN1cnJlbnQgYmFja2VuZCBkb2Vz
IG5vdCBhbHJlYWR5DQpob2xkIGEgY29udGVudCBsb2NrIG9uIHRoZSBidWZmZXIuICBJbiBhIG5v
bi1hc3NlcnQgYnVpbGQgYSBzZWNvbmQgYWNxdWlzaXRpb24NCnByb2NlZWRzLCBhbmQgdGhlIGNv
bnNlcXVlbmNlcyBhcmUgd29yc2UgdGhhbiBhIGZhaWxlZCBhc3NlcnRpb24uDQoNClByaXZhdGVS
ZWZDb3VudERhdGEgaGFzIHJvb20gZm9yIGEgc2luZ2xlIGxvY2ttb2RlLCBzbyBvbmx5IG9uZSBs
b2NrDQphY3F1aXNpdGlvbiBwZXIgYnVmZmVyIHBlciBiYWNrZW5kIGNhbiBiZSB0cmFja2VkLiAg
QSBzZWNvbmQgYWNxdWlzaXRpb24gYWRkcw0KYW5vdGhlciBsb2NrLXZhbHVlIHRvIGJ1Zl9oZHIt
PnN0YXRlIGluIEJ1ZmZlckxvY2tBdHRlbXB0KCksIGJ1dCB0aGUNCmFzc2lnbm1lbnQgb2YgZW50
cnktPmRhdGEubG9ja21vZGUgb3ZlcndyaXRlcyB0aGUgcmVjb3JkIG9mIHRoZSBsb2NrIHRoYXQg
d2FzDQphbHJlYWR5IGhlbGQuICBUaGUgZXZlbnR1YWwgcmVsZWFzZSBzdWJ0cmFjdHMgb25seSBv
bmUgbG9jay12YWx1ZSwgc28gdGhlDQpidWZmZXIncyBsb2NrIHN0YXRlIGlzIGxlZnQgcGVybWFu
ZW50bHkgdG9vIGhpZ2ggYW5kIHRoZSBidWZmZXIgY2FuIG5ldmVyDQphZ2FpbiBiZSBsb2NrZWQg
ZXhjbHVzaXZlbHk6IGFueSBsYXRlciBleGNsdXNpdmUgb3Igc2hhcmUtZXhjbHVzaXZlIHdhaXRl
ciwNCnN1Y2ggYXMgVkFDVVVNLCB3YWl0cyBmb3JldmVyLiAgRXJyb3IgcmVjb3ZlcnkgZG9lcyBu
b3QgaGVscCwgYmVjYXVzZQ0KUmVzT3duZXJSZWxlYXNlQnVmZmVyKCkgYWxzbyByZWxlYXNlcyBh
dCBtb3N0IG9uZSBsb2NrIHBlciBidWZmZXIuDQoNClRoaXMgd2FzIHByZXZpb3VzbHkgcmVwcmVz
ZW50YWJsZTogd2hlbiBjb250ZW50IGxvY2tzIHdlcmUgbHdsb2NrcywgaGVsZA0KbG9ja3Mgd2Vy
ZSB0cmFja2VkIGluIGFuIGFycmF5IChoZWxkX2x3bG9ja3NbXSkgYW5kIExXTG9ja1JlbGVhc2VB
bGwoKQ0KcmVsZWFzZWQgZWFjaCBlbnRyeSwgc28gYSBiYWNrZW5kIHRha2luZyBhIHNoYXJlIGxv
Y2sgdHdpY2Ugb24gdGhlIHNhbWUNCmJ1ZmZlciB3b3JrZWQgYW5kIHJlY292ZXJlZCBjb3JyZWN0
bHkuICBmY2I5Yzk3N2FhNSByZXBsYWNlZCB0aGF0IGFycmF5IHdpdGgNCnRoZSBzaW5nbGUgbG9j
a21vZGUgZmllbGQsIGFuZCAzMzNmNTg2MzcyYSBtYWRlIHRoZSBjb25kaXRpb25hbCBwYXRoIGZh
aWwNCmNsZWFubHkgaW4gdGhpcyBzaXR1YXRpb24sIG5vdGluZyB0aGF0IHRoZXJlIGlzIG5vIHNw
YWNlIHRvIHRyYWNrIG11bHRpcGxlDQpsb2NrIGFjcXVpc2l0aW9ucy4gIFRoZSB1bmNvbmRpdGlv
bmFsIHBhdGggd2FzIGxlZnQgd2l0aCBvbmx5IHRoZSBhc3NlcnRpb24uDQoNClByb21vdGUgaXQg
dG8gYW4gdW5jb25kaXRpb25hbCBlbG9nKEVSUk9SKS4gIExlYWtpbmcgYSBjb250ZW50IGxvY2sg
aXMNCnVucmVjb3ZlcmFibGUgYW5kIGhhcmQgdG8gZGlhZ25vc2UsIHdoZXJlYXMgYW4gZXJyb3Ig
aXMgY29udGFpbmVkIGJ5IHRoZQ0Kbm9ybWFsIGVycm9yIGhhbmRsaW5nLCB3aGljaCByZWxlYXNl
cyB0aGUgb25lIGxvY2sgdGhhdCBpcyByZWNvcmRlZC4gIFRoaXMNCmRvZXMgbm90IGZpeCBhbnkg
Y2FsbGVyIHRoYXQgZG91YmxlLWFjcXVpcmVzOyBpdCBtYWtlcyBzdWNoIGEgYnVnIGZhaWwNCmxv
dWRseSBpbiBwcm9kdWN0aW9uIGluc3RlYWQgb2Ygc2lsZW50bHkgd2VkZ2luZyBhIGJ1ZmZlci4N
Cg0KRGlzY3Vzc2lvbjogaHR0cHM6Ly9wb3N0Z3IuZXMvbS8NCi0tLQ0KIHNyYy9iYWNrZW5kL3N0
b3JhZ2UvYnVmZmVyL2J1Zm1nci5jIHwgMjEgKysrKysrKysrKysrKysrKysrKy0tDQogMSBmaWxl
IGNoYW5nZWQsIDE5IGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pDQoNCmRpZmYgLS1naXQg
YS9zcmMvYmFja2VuZC9zdG9yYWdlL2J1ZmZlci9idWZtZ3IuYyBiL3NyYy9iYWNrZW5kL3N0b3Jh
Z2UvYnVmZmVyL2J1Zm1nci5jDQppbmRleCA1YzgyODY1YTA4NC4uMTRmMGY3YzViZjQgMTAwNjQ0
DQotLS0gYS9zcmMvYmFja2VuZC9zdG9yYWdlL2J1ZmZlci9idWZtZ3IuYw0KKysrIGIvc3JjL2Jh
Y2tlbmQvc3RvcmFnZS9idWZmZXIvYnVmbWdyLmMNCkBAIC01OTMwLDkgKzU5MzAsMjYgQEAgQnVm
ZmVyTG9ja0FjcXVpcmUoQnVmZmVyIGJ1ZmZlciwgQnVmZmVyRGVzYyAqYnVmX2hkciwgQnVmZmVy
TG9ja01vZGUgbW9kZSkNCiAJZW50cnkgPSBHZXRQcml2YXRlUmVmQ291bnRFbnRyeShidWZmZXIs
IHRydWUpOw0KIA0KIAkvKg0KLQkgKiBXZSBiZXR0ZXIgbm90IGFscmVhZHkgaG9sZCBhIGxvY2sg
b24gdGhlIGJ1ZmZlci4NCisJICogV2UgbXVzdCBub3QgYWxyZWFkeSBob2xkIGEgbG9jayBvbiB0
aGlzIGJ1ZmZlci4gIE9ubHkgb25lIGxvY2sNCisJICogYWNxdWlzaXRpb24gcGVyIGJ1ZmZlciBw
ZXIgYmFja2VuZCBjYW4gYmUgdHJhY2tlZCwgYXMNCisJICogUHJpdmF0ZVJlZkNvdW50RGF0YSBo
YXMgcm9vbSBmb3IgYSBzaW5nbGUgbG9ja21vZGU7IHNlZSBhbHNvIHRoZQ0KKwkgKiBjb21tZW50
IGluIEJ1ZmZlckxvY2tDb25kaXRpb25hbCgpLCB3aGljaCBmYWlscyByYXRoZXIgdGhhbiBhY3F1
aXJlIGENCisJICogc2Vjb25kIGxvY2suDQorCSAqDQorCSAqIFdlcmUgd2UgdG8gcHJvY2VlZCwg
QnVmZmVyTG9ja0F0dGVtcHQoKSBiZWxvdyB3b3VsZCBhZGQgYW5vdGhlcg0KKwkgKiBsb2NrLXZh
bHVlIHRvIGJ1Zl9oZHItPnN0YXRlLCBidXQgdGhlIGFzc2lnbm1lbnQgb2YNCisJICogZW50cnkt
PmRhdGEubG9ja21vZGUgZnVydGhlciBkb3duIHdvdWxkIG92ZXJ3cml0ZSB0aGUgcmVjb3JkIG9m
IHRoZQ0KKwkgKiBsb2NrIHdlIGFscmVhZHkgaG9sZC4gIFRoZSBldmVudHVhbCByZWxlYXNlIHdv
dWxkIHRoZW4gc3VidHJhY3Qgb25seQ0KKwkgKiBvbmUgbG9jay12YWx1ZSwgcGVybWFuZW50bHkg
bGVhdmluZyB0aGUgYnVmZmVyJ3MgbG9jayBzdGF0ZSB0b28gaGlnaCwNCisJICogc28gdGhhdCB0
aGUgYnVmZmVyIGNvdWxkIG5ldmVyIGFnYWluIGJlIGxvY2tlZCBleGNsdXNpdmVseS4gIEVycm9y
DQorCSAqIHJlY292ZXJ5IHdvdWxkIG5vdCBzYXZlIHVzIGVpdGhlciwgYXMgUmVzT3duZXJSZWxl
YXNlQnVmZmVyKCkgbGlrZXdpc2UNCisJICogcmVsZWFzZXMgYXQgbW9zdCBvbmUgbG9jayBwZXIg
YnVmZmVyLg0KKwkgKg0KKwkgKiBMZWFraW5nIGEgY29udGVudCBsb2NrIHRoYXQgd2F5IGlzIHVu
cmVjb3ZlcmFibGUgYW5kIGhhcmQgdG8gZGlhZ25vc2UsDQorCSAqIHNvIHJlZnVzZSB0byBkbyBp
dCBldmVuIGluIG5vbi1hc3NlcnQgYnVpbGRzLg0KIAkgKi8NCi0JQXNzZXJ0KGVudHJ5LT5kYXRh
LmxvY2ttb2RlID09IEJVRkZFUl9MT0NLX1VOTE9DSyk7DQorCWlmICh1bmxpa2VseShlbnRyeS0+
ZGF0YS5sb2NrbW9kZSAhPSBCVUZGRVJfTE9DS19VTkxPQ0spKQ0KKwkJZWxvZyhFUlJPUiwgImJ1
ZmZlciAlZCBpcyBhbHJlYWR5IGxvY2tlZCBieSB0aGlzIGJhY2tlbmQiLCBidWZmZXIpOw0KIA0K
IAkvKg0KIAkgKiBMb2NrIG91dCBjYW5jZWwvZGllIGludGVycnVwdHMgdW50aWwgd2UgZXhpdCB0
aGUgY29kZSBzZWN0aW9uIHByb3RlY3RlZA0KLS0gDQoyLjU0LjANCg0K

--b1=_qB0dbzjOgWqZEcVR4yfDCr6wBhsUaLVrEhPjPybuQ--