Re: Reorder shutdown sequence, to flush pgstats later

On 08/01/2025 04:10, Andres Freund wrote:
> I instead opted to somewhat rearrange the shutdown sequence:
>
> This commit changes the shutdown sequence so checkpointer is signalled to
> trigger writing the shutdown checkpoint without terminating it. Once
> checkpointer wrote the checkpoint it will wait for a termination
> signal.
>
> Postmaster now triggers the shutdown checkpoint where we previously did so by
> terminating checkpointer. Checkpointer is terminated after all other children
> have been terminated, tracked using the new PM_SHUTDOWN_CHECKPOINTER PMState.
>
> In addition, the existing PM_SHUTDOWN_2 state is renamed to
> PM_XLOG_IS_SHUTDOWN, that seems a lot more descriptive.

Sounds good.

> This patch is not fully polished, there are a few details I'm not sure about:
>
> - Previously the shutdown checkpoint and process exit were triggered from
> HandleCheckpointerInterrupts(). I could have continued doing so in the
> patch, but it seemed quite weird to have a wait loop in a function called
> HandleCheckpointerInterrupts().
>
> Instead I made the main loop in CheckpointerMain() terminate if checkpointer
> was asked to write the shutdown checkpoint or exit
>
> - In process_pm_pmsignal() the code now triggers a PostmasterStateMachine() if
> checkpointer signalled ShutdownXLOG having completed. We didn't really have
> a need for that before. process_pm_child_exit() does call
> PostmasterStateMachine(), but somehow calling it in process_pm_pmsignal()
> feels slightly off

Looks good to me.

> - I don't love the naming of the various PMState values (existing and new),
> but a larger renaming should probably be done separately?

We currently have PM_WAIT_BACKENDS and PM_WAIT_DEAD_END. Maybe follow
that example and name all the shutdown states after what you're waiting for:

PM_WAIT_BACKENDS, /* waiting for live backends to exit */
PM_WAIT_XLOG_SHUTDOWN, /* waiting for checkpointer to do shutdown
ckpt */
PM_WAIT_XLOG_ARCHIVAL, /* waiting for archiver and walsenders to
finish */
PM_WAIT_DEAD_END, /* waiting for dead-end children to exit */
PM_WAIT_CHECKPOINTER, /* waiting for checkpointer to shut down */
PM_NO_CHILDREN, /* all important children have exited */

> - There's logging in ShutdownXLOG() that's only triggered if called from
> checkpointer. Seems kinda odd - shouldn't we just move that to
> checkpointer.c?

What logging do you mean? The "LOG: shutting down" message? No
objections to moving it, although it doesn't bother me either way.

> The first two patches are just logging improvements that I found helpful to
> write this patch:
>
> 0001: Instead of modifying pmState directly, do so via a new function
> UpdatePMState(), which logs the state transition at DEBUG2.
>
> Requires a helper to stringify PMState.
>
> I've written one-off versions of this patch quite a few times. Without
> knowing in what state postmaster is in, it's quite hard to debug the
> state machine.

+1

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

> 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

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() and just always use DEBUG2
or DEBUG4. Or DEBUG3 as a compromise :-).

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

Nice. A variadic btmask_add() might be handy too.

> And then 0004, the reason for this thread.

Overall, looks good to me.

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