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
