On Fri, Oct 2, 2026 at 7:20 AM Andres Freund <[email protected]> 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 <[email protected]> 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
