Re: Reorder shutdown sequence, to flush pgstats later

Hi,

On Wed, Jan 08, 2025 at 03:10:03PM +0200, Heikki Linnakangas wrote:
> On 08/01/2025 04:10, Andres Freund wrote:
>
> > elog(DEBUG1, "updating PMState from %d/%s to %d/%s",
> > pmState, pmstate_name(pmState), newState, pmstate_name(newState));
>
> I think the state names are enough, no need to print the numerical values.

+1

> > 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.

=== 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 the cases where DEBUG2 or DEBUG4 are used currently are very
> well thought out. To save a few lines of code, I'd be happy with merging
> signal_child_ext() and signal_child()

+1

> > The next is a small prerequisite:
> >
> > 0003: postmaster: Introduce variadic btmask_all_except()
> >
> > I proposed this in another thread, where I also needed
> > btmask_all_except3() but then removed the only call to
> > btmask_all_except2(). Instead of adding/removing code, it seems better
> > to just use a variadic function.

LGTM.

> 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()/?

=== 4

+ * too. That allows checkpointer to perform some last bits of of

Typo "of of"

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.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com