Hi,
Thanks for working on this!
On Wed, 8 Jan 2025 at 22:26, Andres Freund <andres@anarazel.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
