Message-ID: <f809c5b7-340a-44ae-9fb3-b2f1b1c32a7f@iki.fi>
Date: Thu, 6 Mar 2025 01:28:07 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Interrupts vs signals
To: Andres Freund <andres@anarazel.de>
Cc: Thomas Munro <thomas.munro@gmail.com>,
 Michael Paquier <michael@paquier.xyz>, Robert Haas <robertmhaas@gmail.com>,
 Fujii Masao <masao.fujii@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
References: <ZyLQEG1KfCts3N_3@paquier.xyz>
 <b697b96e-78b5-49bb-8d66-581143a65d7a@iki.fi>
 <c272c49d-cd7e-4897-aed7-76dc517607fc@iki.fi>
 <CA+hUKGKs+ozrxt_X2FQ=yv5Mx8ZJ2fB+j8yLVZwa13LedT6CSg@mail.gmail.com>
 <8a0ff52f-de8e-49d2-a3af-ea5bc12f5b97@iki.fi>
 <CA+hUKGKroPrwnUvnN0xORPYvXRY-90PC9g_C4suvp9Cws30yAg@mail.gmail.com>
 <bd961cb2-20fb-40ec-82b0-abcf32a787bb@iki.fi>
 <4ddab51d-e932-40c5-b6fa-18c52db0e082@iki.fi>
 <xa36hiwyk3qdxre56kj3txyd4etttqlliquzirdvofvfnz4svn@bbfvk5uw747b>
 <8a507fb6-df28-49d3-81a5-ede180d7f0fb@iki.fi>
 <jqowm7q5uysdtfrpepn3fsed7d47maoxzaoetymic4ou4w44a6@rvpswde3limw>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: <jqowm7q5uysdtfrpepn3fsed7d47maoxzaoetymic4ou4w44a6@rvpswde3limw>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/f809c5b7-340a-44ae-9fb3-b2f1b1c32a7f%40iki.fi>
Precedence: bulk

On 05/03/2025 21:25, Andres Freund wrote:
> On 2025-02-28 22:24:56 +0200, Heikki Linnakangas wrote:
>> The second patch makes it possible to use ModifyWaitEvent() to switch
>> between WL_POSTMASTER_DEATH and WL_EXIT_ON_PM_DEATH. WaitLatch() used to
>> modify WaitEventSet->exit_on_postmaster_death directly, now it uses
>> ModifyWaitEvent() for that. That's needed because with the final patch,
>> WaitLatch() is in a different source file than WaitEventSet, so it cannot
>> directly modify its field anymore.
> 
> 
>> @@ -505,8 +513,14 @@ WaitLatch(Latch *latch, int wakeEvents, long timeout,
>>   	if (!(wakeEvents & WL_LATCH_SET))
>>   		latch = NULL;
>>   	ModifyWaitEvent(LatchWaitSet, LatchWaitSetLatchPos, WL_LATCH_SET, latch);
>> -	LatchWaitSet->exit_on_postmaster_death =
>> -		((wakeEvents & WL_EXIT_ON_PM_DEATH) != 0);
>> +
>> +	/*
>> +	 * Update the event set for whether WL_EXIT_ON_PM_DEATH or
>> +	 * WL_POSTMASTER_DEATH was requested.  This is also cheap.
>> +	 */
> 
> "for whether" sounds odd to me.
> 
> I'd remove the "also" in the second sentence.

I removed the whole comment. The "also" was referring to the previous 
comment in the function just above what's shown above, but now that I 
look more closely, the other comment already covered the postmaster 
death event.

>> @@ -1037,15 +1051,19 @@ ModifyWaitEvent(WaitEventSet *set, int pos, uint32 events, Latch *latch)
>>   		(!(event->events & WL_LATCH_SET) || set->latch == latch))
>>   		return;
>>   
>> -	if (event->events & WL_LATCH_SET &&
>> -		events != event->events)
>> +	/* Allow switching between WL_POSTMASTER_DEATH and WL_EXIT_ON_PM_DEATH */
>> +	if (event->events & WL_POSTMASTER_DEATH)
> 
> Hm, this only supports switching from WL_POSTMASTER_DEATH to
> WL_EXIT_ON_PM_DEATH, not the other way round, right?

Fixed.

Checking "if (event->events & WL_POSTMASTER_DEATH)" was in fact correct, 
because in a WaitEventSet, WL_EXIT_ON_PM_DEATH is converted to 
WL_POSTMASTER_DEATH with exit_on_postmaster_death = true. But it was 
still broken because when switching from WL_EXIT_ON_PM_DEATH to 
WL_POSTMASTER_DEATH, we took the "events == event->events" quick-exit 
path before reaching this, for the same reason. I fixed that by moving 
this check before the "events == event->events" check.

>>   	{
>> -		elog(ERROR, "cannot modify latch event");
>> +		if (events != WL_POSTMASTER_DEATH && events != WL_EXIT_ON_PM_DEATH)
>> +			elog(ERROR, "cannot modify postmaster death event");
>> +		set->exit_on_postmaster_death = ((events & WL_EXIT_ON_PM_DEATH) != 0);
>> +		return;
>>   	}
> 
> 
>> -	if (event->events & WL_POSTMASTER_DEATH)
>> +	if (event->events & WL_LATCH_SET &&
>> +		events != event->events)
>>   	{
>> -		elog(ERROR, "cannot modify postmaster death event");
>> +		elog(ERROR, "cannot modify latch event");
>>   	}
> 
> Why did you reorder this? I don't have a problem with it, I just can't quite
> follow.

I wanted to keep the non-error returns together for aesthetic reasons, 
that's all. But that point is moot now, because as I mentioned above, I 
had to move the check for switching between WL_POSTMASTER_DEATH and 
WL_EXIT_ON_PM_DEATH anyway.

>> The third patch is mechanical and moves existing code. The file header
>> comments in the modified files are perhaps worth reviewing. They are also
>> just existing text moved around, but there was some small decisions on what
>> exactly should go where.
>>
>> I'll continue working on the other parts, but these patches seems ready for
>> commit already.
> 
> Interestingly git diff's was pretty annoying to read, even with --color-moved,
> unless I used -M100 (setting the rename detection to require 100%
> renamed). With "-M100 --color-moved=dimmed-zebra" it looks a lot saner.

Thanks for the tip! I didn't know about --color-moved.

>> diff --git a/src/backend/storage/ipc/waiteventset.c b/src/backend/storage/ipc/waiteventset.c
>> new file mode 100644
>> index 00000000000..35e836d3398
> 
>> +#include "storage/proc.h"
> 
> proc.h wasn't previously included, and it's not clear to me why it'd be needed
> now? If I remove it, things still compile, and I verified it's not indirectly
> included.
> 
> Perhaps you had WakeupMyProc() in proc.c earlier?

Thanks, removed.

>> +/*
>> + * Wake up my process if it's currently waiting on a WaitEventSet.
> 
> Hm - that's overstating it a bit, I think. If the WES doesn't wait on the
> latch set, this won't wake the process, right?

It does wake up the process from the sleeping syscall in 
WaitEventSetWaitBlock(). WaitEventSetWait() will re-check the wait 
conditions and go back to sleep, if it's not waiting for the latch set.

I think the problem here is that "waiting on a WaitEventSet" is 
ambiguous. I'll change it to "... if it's currently sleeping in 
WaitEventSetWaitBlock()".

>> diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h
>> index 66e7a5b7c08..5af32621c0d 100644
>> --- a/src/include/storage/latch.h
>> +++ b/src/include/storage/latch.h
>> @@ -84,10 +84,11 @@
>>    * use of any generic handler.
>>    *
>>    *
>> - * WaitEventSets allow to wait for latches being set and additional events -
>> - * postmaster dying and socket readiness of several sockets currently - at the
>> - * same time.  On many platforms using a long lived event set is more
>> - * efficient than using WaitLatch or WaitLatchOrSocket.
>> + * See also WaitEventSets in waiteventset.h. They allow to wait for latches
>> + * being set and additional events - postmaster dying and socket readiness of
>> + * several sockets currently - at the same time.  On many platforms using a
>> + * long lived event set is more efficient than using WaitLatch or
>> + * WaitLatchOrSocket.
>>    *
>>    *
>>    * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
>> @@ -102,6 +103,7 @@
>>   
>>   #include <signal.h>
>>   
>> +#include "storage/waiteventset.h"
> 
> Perhaps worth commenting that this is included here for backward compat?

Hmm. You're right that it's not strictly required here. In practice 
though, all callers of WaitLatch() will need to include it, because the 
WL_* argument flags are defined there. I'm not sure if we have a policy 
on that kind of convenience #includes. I'll add a comment.

>> @@ -0,0 +1,95 @@
>> +/*-------------------------------------------------------------------------
>> + *
>> + * waiteventset.h
>> + *		ppoll() / pselect() like interface for waiting for events
>> + *
>> + * WaitEventSets allow to wait for latches being set and additional events -
>> + * postmaster dying and socket readiness of several sockets currently - at the
>> + * same time.  On many platforms using a long lived event set is more
>> + * efficient than using WaitInterrupt or WaitInterruptOrSocket.
> 
> I don't think WaitInterrupt/WaitInterruptOrSocket exist at this stage?

Fixed.

> These minor things aside, this looks good to me.

Thanks! Committed with those fixes and some minor WIN32 build fixes.

-- 
Heikki Linnakangas
Neon (https://neon.tech)