Throwing away unnecessary spin-locks

Hi,

On 2026-10-01 20:50:35 +0100, Alexandre Felipe wrote:

Please try, if you want
$ grep -A 2 -rn SpinLockAcquire src/backend | grep SpinLockRelease -B 2

Story
=====
A common pattern that I have been repeatedly warned against is using
spin-locks unnecessarily. I was surprised to see in checkpointer.c
FirstCallSinceLastCheckpoint.

int new_done;
SpinLockAcquire(&CheckpointerShmem->ckpt_lck);
new_done = CheckpointerShmem->ckpt_done;
SpinLockRelease(&CheckpointerShmem->ckpt_lck);

I think could certainly be replaced by something like

+ pg_compiler_barrier()
+ new_done = CheckpointerShmem->ckpt_done;
+ pg_compiler_barrier()

That's not a correct transformation. A compiler barrier does not guarantee
cache coherency. You actually have to use correct memory barrier pairings. It
might suffice to use a read memory barrier in this case, but obviously the
easiest transformation is to use full memory barriers on both sides.

The main reason for not having changed all of these over is that you actually
have to carefully think about the changes...

Grepping the codebase we get 105 matches, in 26 files.

One interesting case is xlog.c that uses the lock to protect 64-bit
assignments I saw conversations about pg_atomic_u64 for that, but then in
32-bit platforms we get this weird 3-field structure everywhere.

What are you referring to with "weird 3-field structure"?

Greetings,

Andres Freund

On Thu, Oct 1, 2026 at 9:08 PM Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2026-10-01 20:50:35 +0100, Alexandre Felipe wrote:

Please try, if you want
$ grep -A 2 -rn SpinLockAcquire src/backend | grep SpinLockRelease -B 2

Story
=====
A common pattern that I have been repeatedly warned against is using
spin-locks unnecessarily. I was surprised to see in checkpointer.c
FirstCallSinceLastCheckpoint.

int new_done;
SpinLockAcquire(&CheckpointerShmem->ckpt_lck);
new_done = CheckpointerShmem->ckpt_done;
SpinLockRelease(&CheckpointerShmem->ckpt_lck);

I think could certainly be replaced by something like

+ pg_compiler_barrier()
+ new_done = CheckpointerShmem->ckpt_done;
+ pg_compiler_barrier()

That's not a correct transformation. A compiler barrier does not guarantee
cache coherency. You actually have to use correct memory barrier pairings.
It
might suffice to use a read memory barrier in this case, but obviously the
easiest transformation is to use full memory barriers on both sides.

Like this (sometimes I miss C++ templates)

#define SLOCK_DEFINE_SCALAR_ACCESSORS(typename) \

static inline typename \

slock_read_barrier_##typename(volatile slock_t *lock, volatile typename *p)
\

{ \

typename val; \

\

(void) lock; \

AssertPointerAlignment(p, alignof(typename)); \

val = *p; \

pg_read_barrier(); \

return val; \

} \

static inline void \

slock_write_barrier_##typename(volatile slock_t *lock, volatile typename
*p, typename v) \

{ \

(void) lock; \

AssertPointerAlignment(p, alignof(typename)); \

*p = v; \

pg_write_barrier(); \

}

Or on both sides?

The main reason for not having changed all of these over is that you

actually
have to carefully think about the changes...

Or thinking once about conditions that make them possible and apply some
transformation.

Grepping the codebase we get 105 matches, in 26 files.

One interesting case is xlog.c that uses the lock to protect 64-bit
assignments I saw conversations about pg_atomic_u64 for that, but then in
32-bit platforms we get this weird 3-field structure everywhere.

What are you referring to with "weird 3-field structure"?

typedef struct pg_atomic_uint64
{
int sema;
volatile uint64 value;
} pg_atomic_uint64;

Sorry it is not declared as 3 field, I was thinking
struct {int sema; volatile uint32 v1; volatile uint32 v2; }

Regards,
Alexandre

Mutatis mutandis.

At this stage I am happy if it compiles everywhere

On Thu, Oct 1, 2026 at 9:50 PM Alexandre Felipe <
o.alexandre.felipe@gmail.com> wrote:

Hi Alexandre,

#define SLOCK_DEFINE_SCALAR_ACCESSORS(typename) \

static inline typename \

slock_read_barrier_##typename(volatile slock_t *lock, volatile typename

*p) \

{ \

typename val; \

\

(void) lock; \

AssertPointerAlignment(p, alignof(typename)); \

val = *p; \

pg_read_barrier(); \

return val; \

} \

static inline void \

slock_write_barrier_##typename(volatile slock_t *lock, volatile typename

*p, typename v) \

{ \

(void) lock; \

AssertPointerAlignment(p, alignof(typename)); \

*p = v; \

pg_write_barrier(); \

}

On x86 pg_read_barrier() and pg_write_barrier() are only compiler
barriers, so v1 ends up as a plain load and a plain store there. That
hides problems. ARM, RISC-V and POWER are weakly ordered, and on those
a write barrier placed after the store does not order it against the
stores before it. The spinlock version does not have that problem.

I think the safer route is what recent commits like df3978c2340 did,
convert the field to pg_atomic and use pg_atomic_read_membarrier_u32()
and pg_atomic_write_membarrier_u32().

Thanks,
Shihao

On Fri, Oct 2, 2026 at 6:30 AM shihao zhong <zhong950419@gmail.com> wrote:

Hi Alexandre,

#define SLOCK_DEFINE_SCALAR_ACCESSORS(typename) \

static inline typename \

slock_read_barrier_##typename(volatile slock_t *lock, volatile typename

*p) \

{ \

typename val; \

\

(void) lock; \

AssertPointerAlignment(p, alignof(typename)); \

val = *p; \

pg_read_barrier(); \

return val; \

} \

static inline void \

slock_write_barrier_##typename(volatile slock_t *lock, volatile typename

*p, typename v) \

{ \

(void) lock; \

AssertPointerAlignment(p, alignof(typename)); \

*p = v; \

pg_write_barrier(); \

}

On x86 pg_read_barrier() and pg_write_barrier() are only compiler
barriers, so v1 ends up as a plain load and a plain store there. That
hides problems. ARM, RISC-V and POWER are weakly ordered, and on those
a write barrier placed after the store does not order it against the
stores before it. The spinlock version does not have that problem.

pg_write_barrier(), pg_read_barrier(), are compiler barriers, and they
are the same as pg_compiler_barrier()?

Those barriers have no cost at run time right? So it wouldn't hurt
having two.

I think the safer route is what recent commits like df3978c2340 did,

convert the field to pg_atomic and use pg_atomic_read_membarrier_u32()
and pg_atomic_write_membarrier_u32().

I see that pg_atomic_write_membarrier_u32 uses an exchange,
In the patch above I used __atomic_store (that AFAIK will not be reordered)
and might block other processes trying to write on the same
cache line.

Trying to make it portable, to handle at least bool, uint32, uint64, Pointer
in a regular pattern.

Full barrier
pg_memory_barrier();
*out = *in; /* where only one side is shared */
pg_memory_barrier();

Release barrier
pg_compiler_barrier();
*p_shared = v_local;
pg_memory_barrier();

Acquire barrier
pg_memory_barrier();
v_local = *p_shared;
pg_compiler_barrier();

Does it make sense?

Can release/acquire semantics be used or we MUST have a full barrier?

Regards,
Alexandre

Hi Shihao,

On Fri, Oct 2, 2026 at 6:30 AM shihao zhong <zhong950419@gmail.com> wrote:

I think the safer route is what recent commits like df3978c2340 did,
convert the field to pg_atomic and use pg_atomic_read_membarrier_u32()
and pg_atomic_write_membarrier_u32().

Actually, v1 conflicted with that commit, rebasing.

The reason I didn't want to do that is that I wanted a uniform way to handle
all types.

v1.1 pg_atomic_{write,read}_membarrier_uint{32,64} and keeps the
lock for 8- and 16-bits.

Can we use pg_atomic_flag for bool, or will that have different
representations?

Regards,
Alexandre felipe

Hi,

I'm sorry to be blunt, but this dangerous stuff. You're just doing mechanical
replacements, without any analysis. That's a recipe for hard to encounter and
find bugs.

It also doesn't make any sort of sense to me to use slock_* or whatnot. This
has nothing to do with spinlocks.

@@ -2748,9 +2746,7 @@ XLogSetAsyncXactLSN(XLogRecPtr asyncXactLSN)
void
XLogSetReplicationSlotMinimumLSN(XLogRecPtr lsn)
{
-	SpinLockAcquire(&XLogCtl->info_lck);
-	XLogCtl->replicationSlotMinLSN = lsn;
-	SpinLockRelease(&XLogCtl->info_lck);
+	slock_write_uint64(&XLogCtl->info_lck, &XLogCtl->replicationSlotMinLSN, lsn);
}

Just about all the write side ones are completely wrong. This allows the
field to be set while someone else holds the spinlock. As I said before, this
needs careful, documented, analysis FOR EACH AND EVERY SINGLE CHANGE.

This isn't to say that the read sides are right either. Some of them very well
might depend on the read not happening while multiple fields were updated with
the lock held.

Greetings,

Andres Freund

On Fri, Oct 2, 2026 at 7:20 AM Andres Freund <andres@anarazel.de> wrote:

Hi,

I'm sorry to be blunt, but this dangerous stuff. You're just doing
mechanical
replacements, without any analysis. That's a recipe for hard to encounter
and
find bugs.

It also doesn't make any sort of sense to me to use slock_* or whatnot.
This
has nothing to do with spinlocks.

@@ -2748,9 +2746,7 @@ XLogSetAsyncXactLSN(XLogRecPtr asyncXactLSN)
void
XLogSetReplicationSlotMinimumLSN(XLogRecPtr lsn)
{
-     SpinLockAcquire(&XLogCtl->info_lck);
-     XLogCtl->replicationSlotMinLSN = lsn;
-     SpinLockRelease(&XLogCtl->info_lck);
+     slock_write_uint64(&XLogCtl->info_lck,

&XLogCtl->replicationSlotMinLSN, lsn);

}

Just about all the write side ones are completely wrong. This allows the
field to be set while someone else holds the spinlock. As I said before,
this
needs careful, documented, analysis FOR EACH AND EVERY SINGLE CHANGE.

Here is one that looks really broken:
- SpinLockAcquire(&Insert->insertpos_lck);
- current_bytepos = Insert->CurrBytePos;
- SpinLockRelease(&Insert->insertpos_lck);
+ current_bytepos = slock_read_uint64(&Insert->insertpos_lck,
&Insert->CurrBytePos);

The insert itself is guarded via:
SpinLockAcquire(&Insert->insertpos_lck);

startbytepos = Insert->C
startbytepos = Insert->CurrBytePos;
endbytepos = startbytepos + size;
prevbytepos = Insert->PrevBytePos;
Insert->CurrBytePos = endbytepos;
Insert->PrevBytePos = startbytepos;
urrBytePos;
endbytepos = startbytepos + size;
prevbytepos = Insert->PrevBytePos;
Insert->CurrBytePos = endbytepos;
Insert->PrevBytePos = startbytepos;

SpinLockRelease(&Insert->insertpos_lck);

You can't sneak under the lock in this case. The insert lock seems very
dangerous in general. You might have better luck with the info_lock. Have
you measured contention?

merlin

On Fri, Oct 2, 2026 at 7:20 AM Andres Freund <andres@anarazel.de> wrote:

Hi,

I'm sorry to be blunt, but this dangerous stuff. You're just doing
mechanical
replacements, without any analysis. That's a recipe for hard to encounter
and
find bugs.

Hi Andres,

That is a fair point, it doesn't mean that we won't analyse them.

I want to have a high level view and see if this is something we can
mechanise.
And I appreciate all your feedback, this is how I am getting more familiar
with
the hidden rules :)

It also doesn't make any sort of sense to me to use slock_* or whatnot.
This
has nothing to do with spinlocks.

It doesn't use the spin lock, but that is a way to say to the future
readers:
look, this variable is guarded by a spin lock, and we know what we are
doing.

If we simply remove the spin lock in the future someone might look and
wonder
"I think they forgot a lock here! I saw the same variable guarded by a lock
elsewhere."

@@ -2748,9 +2746,7 @@ XLogSetAsyncXactLSN(XLogRecPtr asyncXactLSN)
void
XLogSetReplicationSlotMinimumLSN(XLogRecPtr lsn)
{
-     SpinLockAcquire(&XLogCtl->info_lck);
-     XLogCtl->replicationSlotMinLSN = lsn;
-     SpinLockRelease(&XLogCtl->info_lck);
+     slock_write_uint64(&XLogCtl->info_lck, &XLogCtl->replicationSlotMinLSN,

lsn);

}

Just about all the write side ones are completely wrong. This allows the
field to be set while someone else holds the spinlock. As I said before,
this
needs careful, documented, analysis FOR EACH AND EVERY SINGLE CHANGE.

Noted,
Can I count on you to look at the details?
Is breaking one commit per block and do the analysis at the patch preamble
a good
way to share the analysis?

The analysis will be way longer than the changes themselves.

On Fri, Oct 2, 2026 at 3:49 PM Merlin Moncure <mmoncure@gmail.com> wrote:

Here is one that looks really broken:
- SpinLockAcquire(&Insert->insertpos_lck);
- current_bytepos = Insert->CurrBytePos;
- SpinLockRelease(&Insert->insertpos_lck);
+ current_bytepos = slock_read_uint64(&Insert->insertpos_lck,
&Insert->CurrBytePos);

The insert itself is guarded via:
SpinLockAcquire(&Insert->insertpos_lck);

startbytepos = Insert->C
startbytepos = Insert->CurrBytePos;
endbytepos = startbytepos + size;
prevbytepos = Insert->PrevBytePos;
Insert->CurrBytePos = endbytepos;
Insert->PrevBytePos = startbytepos;
urrBytePos;
endbytepos = startbytepos + size;
prevbytepos = Insert->PrevBytePos;
Insert->CurrBytePos = endbytepos;
Insert->PrevBytePos = startbytepos;

SpinLockRelease(&Insert->insertpos_lck);,

Noted,

Trying to find your snippet, is this the one?
1262 ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos,
XLogRecPtr *PrevPtr)
1263 {
1278 SpinLockAcquire(&Insert->insertpos_lck);
1280 startbytepos = Insert->CurrBytePos;
1282 ptr = XLogBytePosToEndRecPtr(startbytepos);
1290 endbytepos = startbytepos + size;
1303 Insert->CurrBytePos = endbytepos;
1306 SpinLockRelease(&Insert->insertpos_lck);

This illustrates well the cases where things can't be written without a
lock.
atomically. Because changes between line 1280 and line 1303 are rolled back.

However, this particular example doesn't stop us from reading because it
will either
CurBytePos before line 1303, and that is the same as reading under a lock
before that
block, or after the line 1303 and that is equivalent to reading under a
lock after that block.

You can't sneak under the lock in this case. The insert lock seems very

dangerous in general. You might have better luck with the info_lock. Have
you measured contention?

I didn't measure contention. I only run it on my own laptop, which is not
very representative.

Regards,
Alexandre

On Fri, Oct 2, 2026 at 11:02 AM Alexandre Felipe <
o.alexandre.felipe@gmail.com> wrote:

Trying to find your snippet, is this the one?

yeah, there was a copy/paste error, sorry about that

1262 ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos,
XLogRecPtr *PrevPtr)
1263 {
1278 SpinLockAcquire(&Insert->insertpos_lck);
1280 startbytepos = Insert->CurrBytePos;
1282 ptr = XLogBytePosToEndRecPtr(startbytepos);
1290 endbytepos = startbytepos + size;
1303 Insert->CurrBytePos = endbytepos;
1306 SpinLockRelease(&Insert->insertpos_lck);

This illustrates well the cases where things can't be written without a
lock.
atomically. Because changes between line 1280 and line 1303 are rolled
back.

However, this particular example doesn't stop us from reading because it
will either
CurBytePos before line 1303, and that is the same as reading under a lock
before that
block, or after the line 1303 and that is equivalent to reading under a
lock after that block.

ok, sure. This is a bit above my pay grade :-). I guess the point is to
work backward from those kinds of points, measure contention, and show the
benefit.

merlin

Hi,

On 2026-10-02 18:01:59 +0100, Alexandre Felipe wrote:

On Fri, Oct 2, 2026 at 7:20 AM Andres Freund <andres@anarazel.de> wrote:

I'm sorry to be blunt, but this dangerous stuff. You're just doing
mechanical
replacements, without any analysis. That's a recipe for hard to encounter
and
find bugs.

That is a fair point, it doesn't mean that we won't analyse them.

I want to have a high level view and see if this is something we can
mechanise.

Shrug. Then you would not have had to post this, since you could just have
analyzed it before doing so.

And I appreciate all your feedback, this is how I am getting more familiar
with the hidden rules :)

Not rather fundamentally mechanically changing locking code isn't a
particularly hidden rule...

It also doesn't make any sort of sense to me to use slock_* or whatnot.
This
has nothing to do with spinlocks.

It doesn't use the spin lock, but that is a way to say to the future
readers: look, this variable is guarded by a spin lock, and we know what we
are doing.

If we simply remove the spin lock in the future someone might look and
wonder "I think they forgot a lock here! I saw the same variable guarded by
a lock elsewhere."

I don't buy this. I think for most of these the answer would be to move the
relevant state entirely outside of the spinlock protected rule. The whole
point here is to make them *independent* of the spinlock. On both the read
*and* the write side (or have the spinlock on the write side just protect
against independent dangers).

@@ -2748,9 +2746,7 @@ XLogSetAsyncXactLSN(XLogRecPtr asyncXactLSN)
void
XLogSetReplicationSlotMinimumLSN(XLogRecPtr lsn)
{
-     SpinLockAcquire(&XLogCtl->info_lck);
-     XLogCtl->replicationSlotMinLSN = lsn;
-     SpinLockRelease(&XLogCtl->info_lck);
+     slock_write_uint64(&XLogCtl->info_lck, &XLogCtl->replicationSlotMinLSN,

lsn);

}

Just about all the write side ones are completely wrong. This allows the
field to be set while someone else holds the spinlock. As I said before,
this
needs careful, documented, analysis FOR EACH AND EVERY SINGLE CHANGE.

Noted,
Can I count on you to look at the details?

I can't promise that. The code quality and payoff need to be high enough to
make that worthwhile. I'm willing to look some, but I certainly won't have
time to review many rounds of dozens of patches.

Is breaking one commit per block and do the analysis at the patch preamble a
good way to share the analysis?

I'd not do many of these at once. Do one simple conversion, with the relevant
analysis, and see whether it goes anywhere and whether you need to adjust your
approach. After that maybe 2 at once.

I don't think one commit per block is quite right either. You'll typically
have to add comments to the read and write side of some specific state
together, splitting that into separate commits doesn't make sense.

And the code needs to document the rules, not just the commit message.

The analysis will be way longer than the changes themselves.

Of course.

Greetings,

Andres Freund

On Fri, Oct 2, 2026 at 7:05 PM Andres Freund <andres@anarazel.de> wrote:

I'd not do many of these at once. Do one simple conversion, with the
relevant
analysis, and see whether it goes anywhere and whether you need to adjust
your
approach. After that maybe 2 at once.

I don't think one commit per block is quite right either. You'll typically
have to add comments to the read and write side of some specific state
together, splitting that into separate commits doesn't make sense.

And the code needs to document the rules, not just the commit message.

The analysis will be way longer than the changes themselves.

Of course.

Trying the checkpointer, I don't know much about it as a whole, I identified
the logic from the code, combined flags and a version number in a
pg_atomic_u32 state.
the { .ckpt_failed++; .ckpt_done = .ckpt_started } update
handled by inserting a memory barrier, to ensure .ckpt_failed is properly
sampled.

the {.ckpt_flags = 0, .ckpt_started++} was handled by detecting the race
afterwards
read .ckpt_started before and after setting the flags, if it changed, check
the state's
version to determine whether it started before or after RequestCheckpoint
setted
its flags.

Identified something unexpected (to me): RequestCheckpoint can fail due to a
failure on a checkpoint started before checkpointer seeing the updated
flags.
Added a slightly different error message for that condition, just in case.

I imaging that this will not impact performance at all, but I hope it helps
us

Regards,
Alexandre.