Re: Reorder shutdown sequence, to flush pgstats later - Mailing list pgsql-hackers

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