On Wed, 2024-01-17 at 17:33 +0800, Junwang Zhao wrote:
> On Wed, Jan 17, 2024 at 4:54 PM Alvaro Herrera <alvhe...@alvh.no-ip.org>
> wrote:
> > I think this needs more comments. First, in the WaitPMResult enum, we
> > currently have three values -- READY, STILL_STARTING, FAILED. These are
> > all pretty self-explanatory. But this patch adds SHUTDOWN_IN_RECOVERY,
> > and that's not at all self-explanatory. Did postmaster start or not?
> > The enum value's name doesn't make that clear. So I'd do something like
> >
> > typedef enum
> > {
> > POSTMASTER_READY,
> > POSTMASTER_STILL_STARTING,
> > /*
> > * postmaster no longer running, because it stopped after recovery
> > * completed.
> > */
> > POSTMASTER_SHUTDOWN_IN_RECOVERY,
> > POSTMASTER_FAILED,
> > } WaitPMResult;
> >
> > Maybe put the comments in wait_for_postmaster_start instead.
>
> I put the comments in WaitPMResult since we need to add two
> of those if in wait_for_postmaster_start.
I don't think that any comment is needed; the name says it all.
> > Secondly, the patch proposes to add new text to be returned by
> > do_start() when this happens, which would look like this:
> >
> > waiting for server to start... shut down while in recovery
> > update recovery target settings for startup again if needed
> >
> > Is this crystal clear? I'm not sure. How about something like this?
> >
> > waiting for server to start... done, but not running
> > server shut down because of recovery target settings
> >
> > variations on the first phrase:
> >
> > "done, no longer running"
> > "done, but no longer running"
> > "done, automatically shut down"
> > "done, automatically shut down after recovery"
>
> I chose the last one because it gives information to users.
> See V5, thanks for the wording ;)
Why not just leave it at plain "done"?
After all, the server was started successfully.
The second message should make sufficiently clear that the server has stopped.
I didn't like the code duplication introduced by the patch, so I rewrote
that part a bit.
Attached is my suggestion.
I'll set the status to "waiting for author"; if you are fine with my version,
I think that the patch is "ready for committer".
Yours,
Laurenz Albe
From 538c662aa562a5cc7630ac845e1b8a6b4abfffa1 Mon Sep 17 00:00:00 2001
From: Laurenz Albe <laurenz.a...@cybertec.at>
Date: Tue, 9 Jul 2024 21:48:41 +0200
Subject: [PATCH v6] Improve pg_ctl message for shutdown after recovery
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
If the postmaster exits after a successful point-in-time-recovery,
pg_ctl used to report a message that indicated a problem.
Author: Zhao Junwang, Crisp Lee
Reviewed by: Nazir Bilal Yavuz, Nathan Bossart, Ãlvaro Herrera, Laurenz Albe
Discussion: https://postgr.es/m/CAGHPtV7GttPZ-HvxZuYRy70jLGQMEm5%3DLQc4fKGa%3DJ74m2VZbg%40mail.gmail.com
---
src/bin/pg_ctl/pg_ctl.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 405e223c19..324a6da096 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -45,6 +45,7 @@ typedef enum
{
POSTMASTER_READY,
POSTMASTER_STILL_STARTING,
+ POSTMASTER_SHUTDOWN_IN_RECOVERY,
POSTMASTER_FAILED,
} WaitPMResult;
@@ -657,17 +658,23 @@ wait_for_postmaster_start(pid_t pm_pid, bool do_checkpoint)
* On Windows, we may be checking the postmaster's parent shell, but
* that's fine for this purpose.
*/
-#ifndef WIN32
{
+ bool pm_died;
+#ifndef WIN32
int exitstatus;
- if (waitpid(pm_pid, &exitstatus, WNOHANG) == pm_pid)
- return POSTMASTER_FAILED;
- }
+ pm_died = (waitpid(pm_pid, &exitstatus, WNOHANG) == pm_pid);
#else
- if (WaitForSingleObject(postmasterProcess, 0) == WAIT_OBJECT_0)
- return POSTMASTER_FAILED;
+ pm_died = (WaitForSingleObject(postmasterProcess, 0) == WAIT_OBJECT_0);
#endif
+ if (pm_died)
+ {
+ if (get_control_dbstate() == DB_SHUTDOWNED_IN_RECOVERY)
+ return POSTMASTER_SHUTDOWN_IN_RECOVERY;
+ else
+ return POSTMASTER_FAILED;
+ }
+ }
/* Startup still in process; wait, printing a dot once per second */
if (i % WAITS_PER_SEC == 0)
@@ -991,6 +998,10 @@ do_start(void)
progname);
exit(1);
break;
+ case POSTMASTER_SHUTDOWN_IN_RECOVERY:
+ print_msg(_(" done\n"));
+ print_msg(_("server shut down because of recovery target settings\n"));
+ break;
case POSTMASTER_FAILED:
print_msg(_(" stopped waiting\n"));
write_stderr(_("%s: could not start server\n"
--
2.45.2
