Re: Throwing away unnecessary spin-locks - Mailing list pgsql-hackers
| From | Alexandre Felipe |
|---|---|
| Subject | Re: Throwing away unnecessary spin-locks |
| Date | |
| Msg-id | CAE8JnxPmkOvmVWPcycKgCsG9ryQFNZSFS+UR030bQxGC-rZg0A@mail.gmail.com Whole thread |
| In response to | Re: Throwing away unnecessary spin-locks (Merlin Moncure <mmoncure@gmail.com>) |
| Responses |
Re: Throwing away unnecessary spin-locks
Re: Throwing away unnecessary spin-locks |
| List | pgsql-hackers |
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);
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
pgsql-hackers by date: