Re: Reorder shutdown sequence, to flush pgstats later

Hi,

Thanks for working on this!

On Wed, 8 Jan 2025 at 22:26, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> I'm currently to plan the four patches relatively soon, unless somebody speaks
> up, of course. They seem fairly uncontroversial. The renaming of the phases
> doesn't need to wait much longer, I think.
>
> The last two (0006: trigger checkpoints via SetLatch, 0007: change the
> shutdown sequence), probably can use a few more eyes.

Some minor comments:

=== 0001

LGTM.

=== 0002

+ }
+#undef PM_TOSTR_CASE
+ pg_unreachable();
+}

Maybe a blank line after '#undef PM_TOSTR_CASE' (or no blank line at
the end of the pmstate_name() at 0001)?

+ ereport(DEBUG3,
+ (errmsg_internal("sending signal %d/%s to %s process %d",

I am not sure if that makes sense but with the addition of the backend
type here, I think 'sending signal %d/%s to %s process with the pid of
%d' would be clearer.

=== 0003

LGTM.

=== 0004

LGTM.

=== 0005

> I think this is much better than before. I don't love PM_WAIT_XLOG_ARCHIVAL,
> but I can't think of anything better.

I liked this, I think it is good enough.

- PM_SHUTDOWN, /* waiting for checkpointer to do shutdown
+ PM_WAIT_XLOG_SHUTDOWN, /* waiting for checkpointer to do shutdown
* ckpt */

There are couple of variables and functions which include pm_shutdown
in their names:
pending_pm_shutdown_request
handle_pm_shutdown_request_signal()
process_pm_shutdown_request()

Do you think these need to be updated as well?

=== 0006

Please see below.

=== 0007

- * told to shut down. We expect that it wrote a shutdown
- * checkpoint. (If for some reason it didn't, recovery will
- * occur on next postmaster start.)
+ * told to shut down. We know it wrote a shutdown checkpoint,
+ * otherwise we'd still be in PM_SHUTDOWN.

s/PM_SHUTDOWN/PM_WAIT_XLOG_SHUTDOWN/

I have these comments for now. I need to study 0006 and 0007 more.

--
Regards,
Nazir Bilal Yavuz
Microsoft