[PATCH] bufmgr: tighten LWLock:BufferMapping on InvalidateBuffer

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:

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

Is it undocumented feature, or something that should be fixed?

```C
#define MAX_BACKENDS_BITS 18
#define MAX_BACKENDS ((1U << MAX_BACKENDS_BITS)-1)

/* assumes MAX_BACKENDS is a (power of 2) - 1, checked below */
#define LW_VAL_EXCLUSIVE (MAX_BACKENDS + 1)
#define LW_VAL_SHARED 1

/* already (power of 2)-1, i.e. suitable for a mask */
#define LW_SHARED_MASK MAX_BACKENDS
#define LW_LOCK_MASK (MAX_BACKENDS | LW_VAL_EXCLUSIVE)
```

It's documented, not very prominently - see the lwlock.c file header.
LW_VAL_EXCLUSIVE is MAX_BACKENDS + 1, so overflowing needs MAX_BACKENDS + 1
shared holders, and InitializeMaxBackends() won't let MaxBackends get that big.

So there's nothing to fix here.

Best Regards,
Yuhang Qiu.

Thank you Yuhang,

That is correct under the (very reasonable) assumption that a backend will
not hold more than one shared lock of the LWLock.

I also studied the current LWLock the implementation and there is room to
make the no-contention path faster. Is that something worth doing,
or is it pointless?

Regards,
Alexandre

On Mon, Aug 24, 2026 at 8:23 AM Yuhang Qiu <iamqyh@gmail.com> wrote:

Hi,

Please don't top-quote.

On 2026-08-24 09:16:28 +0100, Alexandre Felipe wrote:

I also studied the current LWLock the implementation and there is room to
make the no-contention path faster. Is that something worth doing,
or is it pointless?

Hard to say without knowing what you're actually going to propose. Of couse
making the uncontended path faster is attractive, but correctness / complexity
/ contended performance all are also not uninmportant.

Greetings,

Andres Freund

On Mon, Aug 24, 2026 at 1:25 PM Andres Freund <andres@anarazel.de> wrote:

On 2026-08-24 09:16:28 +0100, Alexandre Felipe wrote:

I also studied the current LWLock the implementation and there is room to
make the no-contention path faster. Is that something worth doing,
or is it pointless?

Hard to say without knowing what you're actually going to propose. Of couse
making the uncontended path faster is attractive, but correctness /
complexity
/ contended performance all are also not uninmportant.

Here I include the experiments. Maybe you don't want to use this
implementation
but it might give you some hints to write a faster version that you like.

There are no algorithmic changes. The changes are about

0002 having two distinct paths for LW_EXCLUSIVE and LW_SHARED,
the rest is diminishing returns. And also added LWLockReleaseLast,
added wrappers as inline functions in lwlock.h that compiles the
current code base replacing the calls
LWLockAcquire(lock, LW_EXCLUSIVE | LW_SHARED) by the
LWLockAcquire(Exclusive|Shared)(lock)

0003 reading the mode from the LWLock state and saving space and time
storing it in the held lwlocks array.
0004 well, bring LWLock(Acquire/Release)X external functions that take
mode as a parameter (a baseline).
0005 Inlined LWLockAttemptLock and folded the loop, placing a single
LWLockAttemptLock at the top of the loop followed by the common logic
(previously at the end bottom of the function), then enqueue and continue
or wait.

Regards,
Alexandre

Hi Alexandre,

Thanks for sharing the experiments. I have a few concerns.

The benchmark runs in a single backend. This mainly measures instruction and
branch overhead. Saving a few nanoseconds here does not necessarily result
in a measurable end-to-end improvement.

The patch also adds more interfaces and makes the locking code harder to use
and maintain. Some changes seem to move costs rather than remove them.

In 0002, LWLockAcquire is declared as a non-static inline function, while its
external definition is removed, when the compiler does not inline it, for
example with -O0, this whill leave an unresolved LWLockAcquire symbol and
causes a link failure:
```C
inline bool LWLockAcquire(LWLock *lock, LWLockMode mode)
```

In 0005, queued is not initialized and is never set to true after queueing,
This can cause repeated queueing, incorrect dequeueing, or prevent the backend
from reaching the normal sleep path:
```C
bool queued;

if (!queued)
{
LWLockQueueSelf(lock, mode);
continue;
}
```

I think it would be more useful to reduce unnecessary LWLock acquisitions or
contention in higher-level code paths, such as buffer mapping and WAL buffers.
For LWLock itself, the important part are atomic operations and cache-line
contention (though there might be no lock contention, but at CPU level, emmm),
wait-list contention, sleep/wakeup mechanism, and fairness...

Best Regards,
Yuhang Qiu.

Hi,

On 2026-08-24 22:01:43 +0100, Alexandre Felipe wrote:

There are no algorithmic changes. The changes are about

0002 having two distinct paths for LW_EXCLUSIVE and LW_SHARED,
the rest is diminishing returns. And also added LWLockReleaseLast,
added wrappers as inline functions in lwlock.h that compiles the
current code base replacing the calls
LWLockAcquire(lock, LW_EXCLUSIVE | LW_SHARED) by the
LWLockAcquire(Exclusive|Shared)(lock)

0003 reading the mode from the LWLock state and saving space and time
storing it in the held lwlocks array.
0004 well, bring LWLock(Acquire/Release)X external functions that take
mode as a parameter (a baseline).
0005 Inlined LWLockAttemptLock and folded the loop, placing a single
LWLockAttemptLock at the top of the loop followed by the common logic
(previously at the end bottom of the function), then enqueue and continue
or wait.

From 7e2fe005525d59c4def5ac0f1060d3d4bfa43351 Mon Sep 17 00:00:00 2001
From: Alexandre Felipe <o.alexandre.felipe@gmail.com>
Date: Mon, 17 Aug 2026 18:19:43 +0100
Subject: [PATCH 1/5] Benchmark

This adds a module for benchmarking LWLocks in a tight loop.

I don't believe that's a particulary interesting test. You don't normally take
a lock to then not do anything when covered by the lock.

From 139f5d67266c94f96f3e88b056c297307efacb32 Mon Sep 17 00:00:00 2001
From: Alexandre Felipe <o.alexandre.felipe@gmail.com>
Date: Mon, 17 Aug 2026 18:41:39 +0100
Subject: [PATCH 2/5] LWLock fast paths

inline LWLockAcquire(LWLock* , LWLockMode)
+- extern LWLockAcquireShared(LWLock*)
|  +- fast LWLockAcquireCommon(LWLock*, LW_SHARED)
|     +- fast LWLockAttemptLock(LWLock*, LW_SHARED)
|     +- ... slow path
+- extern LWLockACquireExclusive(LWLock*)
+- fast LWLockAcquireCommon(LWLock*, LW_EXCLUSIVE)
+- fast LWLockAttemptLock(LWLock*, LW_EXCLUSIVE)
+- ... slow path

LWLock tracking only stores (LWLock*), the mode can be inferred at
any time from the LWLock state. since at any time either the number
of exclusive locks or the number of shared locks on a LWLock must
be zero. If a lock is currently held, one of them will be non-zero.

Is reading the mode from the lock state in the hot path a concern?

Yes, that sounds like a bad idea. The lock state on a somewhat contended lock
tends to bounce very heavily between cores / nodes. Pulling it into shared
state before the atomic operation is a bad idea.

I think that's a clear no-go.

This patch provides a LWLockRelease(LWLock, LWLockMode) that uses
an architecture similar to LWLockAcquire.

inline LWLockReleaseMode(LWLock* , LWLockMode)
+- extern LWLockReleaseShared(LWLock*)
|  +- if(lock is at the top of held_lwlocks)
|  |  fast LWLockReleaseCommon(LWLock*, LW_SHARED)
|  +- ... slow path
+- extern LWLockReleaseExclusive(LWLock*)
+- if(lock is at the top of held_lwlocks)
|  fast LWLockReleaseCommon(LWLock*, LW_EXCLUSIVE)
+- ... slow path

Is the top of the stack check in the hot path a concern?
This patch provides a LWLockReleaseLast(LWLock*, LWLockMode),
that only checks for the existence of a lock.

I doubt the gain of eliding am-i-on-the-top-of-the-stack check is worth it.
To prove that it is you would really need to show in a more realistic scenario
that the gain is worth the interface complexity.

Greetings,

Andres Freund

On Tue, Aug 25, 2026 at 11:37 AM Yuhang Qiu <iamqyh@gmail.com> wrote:

I think it would be more useful to reduce unnecessary LWLock acquisitions
or
contention in higher-level code paths, such as buffer mapping and WAL
buffers.
For LWLock itself, the important part are atomic operations and cache-line
contention (though there might be no lock contention, but at CPU level,
emmm),
wait-list contention, sleep/wakeup mechanism, and fairness...

Experiments show that a LWLockAcquire(...) = 1 is 1000x - 10000x slower
than an
LWLockAcquire(...) = 0. So, what I want to propose here is a more granular
lock.

I see that the locks are already used in partitions, I know that in
BufTable we are
using 128 locks, but it could have millions, we don't do it because of the
overhead, possibly managing waiting lists, and so on.

This patch supports granular LW_EXCLUSIVE locks, the immediate application I
could see is in BufTable, where a lock partition could be further
subdivided in up
to 26 rooms. Shared locks contend to any exclusive lock as today, but
exclusive
locks to different runs can be held simultaneously. (decided to include a
patch
with this before sending).

When the last LW_SHARED is released, all non-overlapping waiters are
awakened
simultaneously, with no need to wait. A LW_EXCLUSIVE might not be put to
sleep
if they don't require the same rooms.

A fairness consideration here is that this implementation doesn't wake up
the
waiters in order. The contending shared locks always lose to uncontended
exclusive locks, this might lead to (1 exclusive) shared (2-exclusive 1
exclusive)*
blocking shared forever. But I hope it serves to illustrate the idea.

If this seems acceptable, the next steps could be (not necessarily in that
order)
(1) adding a PRIORITY bit to mode to control whether it goes before or
after;
(2) don't skip a LW_SHARED if it is first on queue;
(3) accepting rooms on LW_SHARED mode, and promoting uncontended
locks to LW_EXCLUSIVE on the required rooms when the number of waiters is
small.
(4) combining the approaches choosing to release the configuration that
results
in the highest number of waiters being awakened.

I suspect that there are some "Wait for X" cases that I am not handling.

Hi Alexandre,

If this seems acceptable, the next steps could be ...

I don't think this design works.

Rooms change the semantics of an LWLock: exclusive access no longer
excludes all other holders. Data access needs to be reworked to support
this. The buffer mapping access in this patch already has a problem:
different rooms in the same partition can map to the same hash bucket,
allowing concurrent modifications to its chain and causing entries to be
lost.

In 0001, releasing the last room while LW_FLAG_WAKE_IN_PROGRESS is set
leaves LW_EXCLUSIVE set. Shared waiters cannot clear it and remain
blocked forever unless a subsequent exclusive acquisition and release
clears it. This also affects ordinary LW_EXCLUSIVE acquisitions.

All 26 rooms share one state word. Acquirers of disjoint rooms still
CAS on the same cache line, so the scalability bottleneck remains and
may even get worse. In this case, I would increase NUM_BUFFER_PARTITIONS.

Best regards,
Yuhang Qiu