Re: Throwing away unnecessary spin-locks - Mailing list pgsql-hackers

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