Re: Reorder shutdown sequence, to flush pgstats later

Hi,

On 2025-01-08 14:48:21 +0000, Bertrand Drouvot wrote:
> On Wed, Jan 08, 2025 at 03:10:03PM +0200, Heikki Linnakangas wrote:
> > On 08/01/2025 04:10, Andres Freund wrote:
> > > 0002: Make logging of postmaster signalling child processes more consistent
> > >
> > >        I found it somewhat hard to understand what's happening during state
> > >        changes without being able to see the signals being sent. While we did
> > >        have logging in SignalChildren(), we didn't in signal_child(), and most
> > >        signals that are important for the shutdown sequence are sent via
> > >        signal_child().
> >
> > +1
>
> Random comments:
>
> === 1
>
> +static const char *
> +pm_signame(int signal)
> +{
> +#define PM_CASE(state) case state: return #state
> +       switch (signal)
>
> What about?
> "
> #define PM_SIGNAL_CASE(sig) case sig: return #sig
> "
> to better reflect its context.

I went for PM_TOSTR_CASE - that way the same name can be used in
pmstate_name().


> === 2
>
> +               default:
> +                       /* all signals sent by postmaster should be listed here */
> +                       Assert(false);
> +                       return "(unknown)";
> +       }
> +#undef PM_CASE
> +       pg_unreachable();
>
> Shouldn't we remove the "return "(unknown)"? (if not the pg_unreachable() looks
> like dead code).

I don't think so - we're not listing all signals here, just ones that
postmaster is currently using. They're also typically not defined in an enum
allowing the compiler to warn about uncovered values. It seemed better to
actually return something in a production build rather than aborting.

Some compilers tend to be pretty annoying about missing return values, that's
why I added the pg_unreachable(). Perhaps I should have done a
return "" /* silence compilers */;

or such.



> > Nice. A variadic btmask_add() might be handy too.
> >
> > > And then 0004, the reason for this thread.
>
> Did not give it a detailled review, but IIUC it now provides this flow:
>
> PM_SHUTDOWN->PM_XLOG_IS_SHUTDOWN->PM_WAIT_DEAD_END->PM_SHUTDOWN_CHECKPOINTER->PM_NO_CHILDREN
>
> and that looks good to me to fix the issue.
>
> A few random comments:
>
> === 3
>
> + postmaster.c will only
> + * signal checkpointer to exit after all processes that could emit stats
> + * have been shut down.
>
> s/postmaster.c/PostmasterStateMachine()/?

I just went for 'postmaster' (without the .c) - I don't think it's really
useful to specifically reference s/postmaster.c/PostmasterStateMachine() in
checkpointer.c. But I could be swayed if you feel strongly.

> === 4
>
> + * too. That allows checkpointer to perform some last bits of of
>
> Typo "of of"

Fixed.


> I'll give 0004 a closer look once it leaves the WIP state but want to share that
> it looks like a good way to fix the issue.

Cool.

Thanks for the review,

Andres Freund