Re: Interrupts vs signals

On 10/07/2024 09:48, Thomas Munro wrote:
> On Mon, Jul 8, 2024 at 9:38 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> The patch actually does both: it still does kill(SIGUSR1) and also sets
>> the latch.
> 
> Yeah, I had some ideas about supporting old extension code that really
> wanted a SIGUSR1, but on reflection, the only reason anyone ever wants
> that is so that sigusr1_handler can SetLatch(), which pairs with
> WaitLatch() in WaitForBackgroundWorker*().  Let's go all the way and
> assume that.

+1

>> I think it would be nice if RegisterDynamicBackgroundWorker() had a
>> "bool notify_me" argument, instead of requiring the caller to set
>> "bgw_notify_pid = MyProcPid" before the call. That's a
>> backwards-compatibility break, but maybe we should bite the bullet and
>> do it. Or we could do this in RegisterDynamicBackgroundWorker():
>>
>> if (worker->bgw_notify_pid == MyProcPid)
>>       worker->bgw_notify_pgprocno = MyProcNumber;
>>
>> I think we can forbid setting pgw_notify_pid to anything else than 0 or
>> MyProcPid.
> 
> Another idea: we could keep the bgw_notify_pid field around for a
> while, documented as unused and due to be removed in future.  We could
> automatically capture the caller's proc number.  So you get latch
> wakeups by default, which I expect many people want, and most people
> could cope with even if they don't want them.  If you really really
> don't want them, you could set a new flag BGW_NO_NOTIFY.

Ok. I was going to say that it feels excessive to change the default 
like that. However, searching for RegisterDynamicBackgroundWorker() in 
github, I can't actually find any callers that don't set pg_notify_pid. 
So yeah, make sense.

> I have now done this part of the change in a new first patch.  This
> particular use of kill(SIGUSR1) is separate from the ProcSignal
> removal, it's just that it relies on ProcSignal's handler's default
> action of calling SetLatch().  It's needed so the ProcSignal-ectomy
> can fully delete sigusr1_handler(), but it's not directly the same
> thing, so it seemed good to split the patch.

PostmasterMarkPIDForWorkerNotify() is now unused. Which means that 
bgworker_notify is never set, and BackgroundWorkerStopNotifications() is 
never called either.

>> A SetLatchRobust would be nice. Looking at SetLatch(), I don't think it
>> can do any damage if you called it on a pointer to garbage, except if
>> the pointer itself is bogus, then just dereferencing it an cause a
>> segfault. So it would be nice to have a version specifically designed
>> with that in mind. For example, it could assume that the Latch's pid is
>> never legally equal to MyProcPid, because postmaster cannot own any latches.
> 
> Yeah I'm starting to think that all we need to do here is range-check
> the proc number.  Here's a version that adds:
> ProcSetLatch(proc_number).  Another idea would be for SetLatch(latch)
> to sanitise the address of a latch, ie its offset and range.

Hmm, I don't think postmaster should trust ProcGlobal->allProcCount either.

> The next problems to remove are, I think, the various SIGUSR2, SIGINT,
> SIGTERM signals sent by the postmaster.  These should clearly become
> SendInterrupt() or ProcSetLatch().

+1

> The problem here is that the
> postmaster doesn't have the proc numbers yet.  One idea is to teach
> the postmaster to assign them!  Not explored yet.

I've been thinking that we should:

a) assign every child process a PGPROC entry, and make postmaster 
responsible for assigning them like you suggest. We'll need more PGPROC 
entries, because currently a process doesn't reserve one until 
authentication has happened. Or we change that behavior.

or

b) Use the pmsignal.c slot numbers for this, instead of ProcNumbers. 
Postmaster already assigns those.

I'm kind of leaning towards b) for now, because it feels like a much 
smaller patch. In the long run, it would be nice if every child process 
had a ProcNumber, though. It was a nice simplification in v17 that we 
don't have separate BackendId and ProcNumber anymore; similarly it would 
be nice to not have separate PMChildSlot and ProcNumber concepts.

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