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

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