Re: Using WaitEventSet in the postmaster

On Sun, Jan 8, 2023 at 11:55 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2023-01-07 18:08:11 +1300, Thomas Munro wrote:
> > On Sat, Jan 7, 2023 at 12:25 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > On 2023-01-07 11:08:36 +1300, Thomas Munro wrote:
> > > > 3. Is it OK to clobber the shared pending flag for SIGQUIT, SIGTERM,
> > > > SIGINT? If you send all of these extremely rapidly, it's
> > > > indeterminate which one will be seen by handle_shutdown_request().
> > >
> > > That doesn't seem optimal. I'm mostly worried that we can end up downgrading a
> > > shutdown request.
> >
> > I was contemplating whether I needed to do some more push-ups to
> > prefer the first delivered signal (instead of the last), but you're
> > saying that it would be enough to prefer the fastest shutdown type, in
> > cases where more than one signal was handled between server loops.
> > WFM.
>
> I don't see any need for such an order requirement - in case of receiving a
> "less severe" shutdown request first, we'd process the more severe one soon
> after. There's nothing to be gained by trying to follow the order of the
> incoming signals.

Oh, I fully agree. I was working through the realisation that I might
need to serialise the handlers to implement the priority logic
correctly (upgrades good, downgrades bad), but your suggestion
fast-forwards to the right answer and doesn't require blocking, so I
prefer it, and had already gone that way in v9. In this version I've
added a comment to explain that the outcome is the same in the end,
and also fixed the flag clearing logic which was subtly wrong before.

> > > I wonder if it'd be good to have a _pm_ in the name.
> >
> > I dunno about this one, it's all static stuff in a file called
> > postmaster.c and one (now) already has pm in it (see below).
>
> I guess stuff like signal handlers and their state somehow seems more global
> to me than their C linkage type suggests. Hence the desire to be a bit more
> "namespaced" in their naming. I do find it somewhat annoying when reasonably
> important global variables aren't uniquely named when using a debugger...

Alright, renamed.

> A few more code review comments:
>
> DetermineSleepTime() still deals with struct timeval, which we maintain at
> some effort. Just to then convert it away from struct timeval in the
> WaitEventSetWait() call. That doesn't seem quite right, and is basically
> introduced in this patch.

I agree, but I was trying to minimise the patch: signals and events
stuff is a lot already. I didn't want to touch DetermineSleepTime()'s
time logic in the same commit. But here's a separate patch for that.

> I think ServerLoop still has an outdated comment:
>
> *
> * NB: Needs to be called with signals blocked

Fixed.

> > + /* Process work scheduled by signal handlers. */
>
> Very minor: It feels a tad off to say that the work was scheduled by signal
> handlers, it's either from other processes or by the OS. But ...

OK, now it's "requested via signal handlers".

> > +/*
> > + * Child processes use SIGUSR1 to send 'pmsignals'. pg_ctl uses SIGUSR1 to ask
> > + * postmaster to check for logrotate and promote files.
> > + */
>
> s/send/notify us of/, since the concrete "pmsignal" is actually transported
> outside of the "OS signal" level?

Fixed.

> LGTM.

Thanks. Here's v10. I'll wait a bit longer to see if anyone else has feedback.