Re: Throwing away unnecessary spin-locks

On Fri, Oct 2, 2026 at 7:20 AM Andres Freund <andres(at)anarazel(dot)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