[PATCH] bufmgr: tighten LWLock:BufferMapping on InvalidateBuffer
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253380psql -h localhost -U postgresBuilt from patchset v1 (message #1), August 23, 2026 at 12:13 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253380_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253380_1 && git checkout t253380_1Patchset v1 (message #1) is on t253380_1
While reviewing another thread [1]/messages/by-id/dbbd1998-19ff-4ac2-b4b1-a39f4ec1b0f5@iki.fi, I noticed a metion to lock duration,
and I think there is room to improve the current implementation on
InvalidateBuffer
and InvalidateVictimBuffer, that thread is already a fork of something else,
but I think this has its own merit.
In my (current) understanding partition LWLock guards a fraction of the
hash table
entries, and the buffer header lock as the name suggests guards the buffer
descriptor.
I think holding the Buffer header lock is not a problem,
as contention would require someone else accessing precisely that buffer,
holding the partition lock is more critical, as it may contend on thousands
of
distinct buffers.
This patch minimises the partition time, and keep the buffer header
guarding
the largest portion of the function body.
[1]: /messages/by-id/dbbd1998-19ff-4ac2-b4b1-a39f4ec1b0f5@iki.fi
/messages/by-id/dbbd1998-19ff-4ac2-b4b1-a39f4ec1b0f5@iki.fi
Hi,
On 2026-08-11 19:15:48 +0100, Alexandre Felipe wrote:
While reviewing another thread [1], I noticed a metion to lock duration,
and I think there is room to improve the current implementation on
InvalidateBuffer
and InvalidateVictimBuffer, that thread is already a fork of something else,
but I think this has its own merit.In my (current) understanding partition LWLock guards a fraction of the
hash table
entries, and the buffer header lock as the name suggests guards the buffer
descriptor.
I think holding the Buffer header lock is not a problem,
as contention would require someone else accessing precisely that buffer,
holding the partition lock is more critical, as it may contend on thousands
of
distinct buffers.This patch minimises the partition time, and keep the buffer header
guarding
the largest portion of the function body.
You absolutely, definitely, never, ever ever may hold a spinlock over
something even remotely as complicated like an lwlock or a hashtable
delete. NEVER.
And without that the patch is completely unsafe.
Greetings,
Andres Freund
On Tue, Aug 11, 2026, 20:26 Alexandre Felipe <o.alexandre.felipe@gmail.com>
wrote:
Show quoted text
On Tue, Aug 11, 2026 at 7:47 PM Andres Freund <andres@anarazel.de> wrote:
You absolutely, definitely, never, ever ever may hold a spinlock over
something even remotely as complicated like an lwlock or a hashtable
delete. NEVER.So, the problem of spin locks there is that the contenders will spend
CPU on this loop right?
while (old_buf_state & BM_LOCKED)
{
perform_spin_delay(&delayStatus);
old_buf_state = pg_atomic_read_u64(&desc->state);
}
And since there is a perform_spin_delay there I assume it is not 100%
I imagined this was tuned to use maybe 1% CPU.If not, can't we simply use an exponentially increasing sleep until we get
to say 1ms and that uses virtually 0 CPU while waiting.And without that the patch is completely unsafe.
And with that is it safe? Pretending LockBufHdr not to be a spin-lock.
Thank you for your quick response Andres
Regards,
Alexandre
Import Notes
Reply to msg id not found: CAE8JnxN38Ms3N+2NAVk+hoNiZty4hLAWajNo1nitCXhhjqAbvA@mail.gmail.com
Hi,
On 2026-08-11 20:32:40 +0100, Alexandre Felipe wrote:
On Tue, Aug 11, 2026, 20:26 Alexandre Felipe <o.alexandre.felipe@gmail.com>
wrote:On Tue, Aug 11, 2026 at 7:47 PM Andres Freund <andres@anarazel.de> wrote:
You absolutely, definitely, never, ever ever may hold a spinlock over
something even remotely as complicated like an lwlock or a hashtable
delete. NEVER.So, the problem of spin locks there is that the contenders will spend
CPU on this loop right?
while (old_buf_state & BM_LOCKED)
{
perform_spin_delay(&delayStatus);
old_buf_state = pg_atomic_read_u64(&desc->state);
}
And since there is a perform_spin_delay there I assume it is not 100%
I imagined this was tuned to use maybe 1% CPU.If not, can't we simply use an exponentially increasing sleep until we get
to say 1ms and that uses virtually 0 CPU while waiting.
It already uses exponential backoff, via perform_spin_delay().
I went into some detail why this is not ok in
/messages/by-id/bk5cbayf3q4yll4fqs2wbmonzhykg43k2k5v37vokkko53ebsd@spcp4mxlb7hb
This specific change looks like it'd be flat out broken even without the
spinlock error recovery concerns, due to causing undetected deadlocks. You
can't just change the nesting of locks in one place.
Greetings,
Andres Freund
Hi Hackers,
Scrolling lwlock.c I noticed this piece of code in LWLockAttemptLock
if (mode == LW_EXCLUSIVE)
{
lock_free = (old_state & LW_LOCK_MASK) == 0;
if (lock_free)
desired_state += LW_VAL_EXCLUSIVE;
}
else
{
lock_free = (old_state & LW_VAL_EXCLUSIVE) == 0;
if (lock_free)
desired_state += LW_VAL_SHARED;
}
Where LW_LOCK_MASK is [xsss...ss], i.e. one bit for exclusive lock
followed by multiple bits for shared locks.
When acquiring shared locks LW_VAL_EXCLUSIVE times, we end up
with an exclusive lock because the shared lock count overflows to the
shared lock [0111...11] + [0000...01] = [1000..00].
That is probably fine, it will stop us from getting exclusive locks or any
additional shared locks.
The mode is stored as LW_SHARED
held_lwlocks[num_held_lwlocks++].mode = mode;
So the release will revert the counter correctly
mode = held_lwlocks[i].mode;
...
if (mode == LW_EXCLUSIVE)
oldstate = pg_atomic_sub_fetch_u32(&lock->state, LW_VAL_EXCLUSIVE);
else
oldstate = pg_atomic_sub_fetch_u32(&lock->state, LW_VAL_SHARED);
But the LWLock state of LW_VAL_EXCLUSIVE shared locks is identical to
an LWLock with one exclusive lock.
I didn't see (so far) mentions of this (clever?) behaviour.
Is it undocumented feature, or something that should be fixed?
That would be a one line fix under the current assumptions
(old_state & LW_LOCK_MASK) + LW_VAL_SHARED < LW_VAL_EXCLUSIVE;
Regards,
Alexandre