Re: Reorder shutdown sequence, to flush pgstats later

Hi,

On Wed, Jan 08, 2025 at 02:26:15PM -0500, Andres Freund 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.

Thanks for the patches.

A few comments:

0001 LGTM.

0002,

=== 1

+static const char *
+pm_signame(int signal)
+{
+#define PM_TOSTR_CASE(state) case state: return #state
+   switch (signal)

s/state/signal/? (seems better in the pm_signame() context)

0003 and 0004 LGTM.

0005,

=== 2

+ PM_WAIT_XLOG_ARCHIVAL,          /* waiting for archiver and walsenders to

> I don't love PM_WAIT_XLOG_ARCHIVAL, but I can't think of anything better.

PM_WAIT_ARCHIVER_WALSENDERS maybe? (that would follow the pattern of naming
the processes like PM_WAIT_BACKENDS, PM_WAIT_CHECKPOINTER for example).

That said, I'm not 100% convinced it makes it more clear though...

> The last two (0006: trigger checkpoints via SetLatch, 0007: change the
> shutdown sequence), probably can use a few more eyes.

0006,

=== 3

+        * when it does start, with or without getting a signal.

s/getting a signal/getting a latch set/ or "getting notified"?

=== 4

+  if (checkpointerProc == INVALID_PROC_NUMBER)
   {
      if (ntries >= MAX_SIGNAL_TRIES || !(flags & CHECKPOINT_WAIT))
      {
          elog((flags & CHECKPOINT_WAIT) ? ERROR : LOG,
-               "could not signal for checkpoint: checkpointer is not running");
+                "could not notify checkpoint: checkpointer is not running");

Worth renaming MAX_SIGNAL_TRIES with MAX_NOTIFY_TRIES then?

0007,

=== 5

+       pqsignal(SIGINT, ReqShutdownXLOG);

Worth a comment like:

pqsignal(SIGINT, ReqShutdownXLOG);  /* trigger shutdown checkpoint */


=== 6

+ * Wait until we're asked to shut down. By seperating the writing of the

Typo: s/seperating/separating/

I don't really anything else in 0006 and 0007 but as you said it's probably
worth a few more eyes as the code change is pretty substantial.

Regards,

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