Subject: Re: Spinlock can be released twice in procsignal.c

From: Michael Paquier <[email-masked]>
Date: Wed, 26 Feb 2025 13:56:13 +0900

On Wed, Feb 26, 2025 at 09:08:53AM +0500, Andrey Borodin wrote:
> Looks like the oversight in 9d9b9d4. IMO the fix is correct.

 	if (pg_atomic_read_u32(&slot->pss_pid) != 0)
 	{
-		SpinLockRelease(&slot->pss_mutex);
 		elog(LOG, "process %d taking over ProcSignal slot %d, but it's not empty",
 			 MyProcPid, MyProcNumber);
 	}

This fix is not correct.  No system function calls (well basically
most of them) or even more no PostgreSQL-specific calls should happen
while holding a spinlock.  elog() is a good example of what not to do.
One example: imagine a palloc failure while holding this spinlock in
this elog().

The code should be restructured so as we read pss_pid while holding
the spinlock, release the spinlock, and then LOG.  Based on the
structure of this routine, you could just assign a boolean to decide
if something should be logged and delay the LOG until the spinlock is
released, because we don't intend an early exit like in
CleanupProcSignalState().  In ~16, ProcSignalInit() is able to LOG
things early, but the ProcSignal is forced even if pss_pid is set, so
delaying the LOG to be generated after updating ProcSignal does not
matter.
--
Michael

---

From: Maxim Orlov <[email-masked]>
Date: Wed, 26 Feb 2025 09:57:45 +0300

> One example: imagine a palloc failure while holding this spinlock in

Indeed. PFA the correct one.

-- 
Best regards,
Maxim Orlov.

---

From: Andrey Borodin <[email-masked]>
Date: Wed, 26 Feb 2025 13:25:49 +0500

Michael, thanks a lot for the detailed explanation.

> On 26 Feb 2025, at 11:57, Maxim Orlov <[email-masked]> wrote:
> 
> <v2-0001-Avoid-double-spinlock-release.patch>

I’d suggest to give some more descriptive name to “logit” and expand comment “/* sanity check */“. This comment was easier to understand when elog() was near, but now IMO we can have few words about what is going on.

Best regards, Andrey Borodin.

---

From: Maxim Orlov <[email-masked]>
Date: Wed, 26 Feb 2025 12:33:16 +0300

On Wed, 26 Feb 2025 at 11:26, Andrey Borodin <[email-masked]> wrote:

>
> I’d suggest to give some more descriptive name to “logit” and expand
> comment “/* sanity check */“. This comment was easier to understand when
> elog() was near, but now IMO we can have few words about what is going on.

Done. Except for a new name for "logit" variable. Unfortunately, I can't
think of anything sane. As an example I looked at sequence.c. The same name
is used there. I will gladly change this name to whatever you want if it
still look misleading or incomplete for you in some way or another. Just
write the name that you think is correct.

-- 
Best regards,
Maxim Orlov.

---

From: Michael Paquier <[email-masked]>
Date: Thu, 27 Feb 2025 09:46:49 +0900

> write the name that you think is correct.

"old_pss_pid", because this is an old pss_pid value from a previous
slot.  Keeping the value around after releasing the slot is also
useful for debugging, so rather than a boolean it is possible to use a
uint32.  The "sanity check" comment was also a bit out-of-order
because it was written just before acquiring the spinlock.

Tweaked a bit the whole, and the result looked OK so applied on HEAD.
--
Michael

---

From: Maxim Orlov <[email-masked]>
Date: Thu, 27 Feb 2025 09:11:12 +0300

Great! Looks very good. Thanks a lot!

-- 
Best regards,
Maxim Orlov.