Re: make pg_ctl more friendly
On 2024/07/19 2:58, Tom Lane wrote: Fujii Masao writes: "because of recovery target settings" isn't always accurate. For example, if the DBA shuts down the server during recovery, POSTMASTER_SHUTDOWN_IN_RECOVERY can be returned regardless of the recovery target settings. Should we change the message to something like "server shut down in recovery" for accuracy? Hmm, I just pushed it with Laurenz's wording. I don't mind if we change it again, but I'm not sure that there's much wrong with it as it stands. Keep in mind that the context is the DBA doing "pg_ctl start". It seems unlikely that he/she would concurrently do "pg_ctl stop". Even if that did happen, do we really need to phrase the message to account for it? I like Laurenz's wording because it points the user in the direction of the settings that would need adjustment if an immediate shutdown wasn't what was expected/wanted. If we just say "shut down in recovery", that may be accurate, but it offers little help as to what to do next. I was thinking the scenario where "pg_ctl -w start" exits due to a recovery target setting, especially with recovery_target_action=shutdown, can happen not so many times. This is because the server typically can reach PM_STATUS_READY or PM_STATUS_STANDBY, and pg_ctl exits normally before the recovery target is reached. On the other thand, if users start the crash recovery and find misconfiguration of parameter requiring a server restart, they might shut down the server during recovery to fix it. In this case, mentioning "recovery target" could be confusing. This scenario also might not be so common, but seems a bit more likely than the recovery target case. I understand this might be a minority opinion, though.. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Re: make pg_ctl more friendly
Fujii Masao writes: > "because of recovery target settings" isn't always accurate. > For example, if the DBA shuts down the server during recovery, > POSTMASTER_SHUTDOWN_IN_RECOVERY can be returned regardless of > the recovery target settings. Should we change the message to > something like "server shut down in recovery" for accuracy? Hmm, I just pushed it with Laurenz's wording. I don't mind if we change it again, but I'm not sure that there's much wrong with it as it stands. Keep in mind that the context is the DBA doing "pg_ctl start". It seems unlikely that he/she would concurrently do "pg_ctl stop". Even if that did happen, do we really need to phrase the message to account for it? I like Laurenz's wording because it points the user in the direction of the settings that would need adjustment if an immediate shutdown wasn't what was expected/wanted. If we just say "shut down in recovery", that may be accurate, but it offers little help as to what to do next. regards, tom lane
Re: make pg_ctl more friendly
Junwang Zhao writes: > On Wed, Jul 10, 2024 at 3:59 AM Laurenz Albe wrote: >> Attached is my suggestion. > The patch LGTM. >> 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". > I've set it to "ready for committer", thanks. Pushed, thanks. regards, tom lane
Re: make pg_ctl more friendly
On 2024/07/10 11:45, Junwang Zhao wrote:
Attached is my suggestion.
The patch LGTM.
+ case POSTMASTER_SHUTDOWN_IN_RECOVERY:
+ print_msg(_(" done\n"));
+ print_msg(_("server shut down because of recovery
target settings\n"));
"because of recovery target settings" isn't always accurate.
For example, if the DBA shuts down the server during recovery,
POSTMASTER_SHUTDOWN_IN_RECOVERY can be returned regardless of
the recovery target settings. Should we change the message to
something like "server shut down in recovery" for accuracy?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Re: make pg_ctl more friendly
Hi, Laurenz
On Wed, Jul 10, 2024 at 3:59 AM Laurenz Albe wrote:
>
> On Wed, 2024-01-17 at 17:33 +0800, Junwang Zhao wrote:
> > On Wed, Jan 17, 2024 at 4:54 PM Alvaro Herrera
> > 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.
The patch LGTM.
>
> 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".
I've set it to "ready for committer", thanks.
>
> Yours,
> Laurenz Albe
--
Regards
Junwang Zhao
Re: make pg_ctl more friendly
On Wed, 2024-01-17 at 17:33 +0800, Junwang Zhao wrote:
> On Wed, Jan 17, 2024 at 4:54 PM Alvaro Herrera
> 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
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, , WNOHANG) == pm_pid)
-return POSTMASTER_FAILED;
- }
+ pm_died = (waitpid(pm_pid, , 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
Re: make pg_ctl more friendly
Hi Alvaro,
On Wed, Jan 17, 2024 at 4:54 PM Alvaro Herrera 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.
>
> 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
Agreed.
>
> 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 ;)
>
> --
> Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
> "Now I have my system running, not a byte was off the shelf;
> It rarely breaks and when it does I fix the code myself.
> It's stable, clean and elegant, and lightning fast as well,
> And it doesn't cost a nickel, so Bill Gates can go to hell."
--
Regards
Junwang Zhao
v5-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch
Description: Binary data
Re: make pg_ctl more friendly
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.
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"
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Now I have my system running, not a byte was off the shelf;
It rarely breaks and when it does I fix the code myself.
It's stable, clean and elegant, and lightning fast as well,
And it doesn't cost a nickel, so Bill Gates can go to hell."
Re: make pg_ctl more friendly
Hi Nathan,
On Tue, Jan 16, 2024 at 5:39 AM Nathan Bossart wrote:
>
> + POSTMASTER_RECOVERY_SHUTDOWN,
>
> Perhaps this should be POSTMASTER_SHUTDOWN_IN_RECOVERY to match the state
> in the control file?
Agreed
>
> + case POSTMASTER_RECOVERY_SHUTDOWN:
> + print_msg(_("PITR shutdown\n"));
> + print_msg(_("update configuration for startup
> again if needed\n"));
> + break;
>
> I'm not sure I agree that this is a substantially friendlier message. From
> a quick skim of the thread, it seems like you want to avoid sending a scary
> error message if Postgres was intentionally shut down while in recovery.
> If I got this particular message, I think I would be worried that something
> went wrong during my point-in-time restore, and I'd be scrambling to figure
> out what configuration this message wants me to update.
>
> If I'm correctly interpreting the intent here, it might be worth fleshing
> out the messages a bit more. For example, instead of "PITR shutdown,"
> perhaps we could say "shut down while in recovery."
Make sense. Fixed. See V4
> And maybe we should
> point to the specific settings in the latter message.
I've changed this latter message to:
update recovery target settings for startup again if needed
What do you think?
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
--
Regards
Junwang Zhao
v4-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch
Description: Binary data
Re: make pg_ctl more friendly
+ POSTMASTER_RECOVERY_SHUTDOWN,
Perhaps this should be POSTMASTER_SHUTDOWN_IN_RECOVERY to match the state
in the control file?
+ case POSTMASTER_RECOVERY_SHUTDOWN:
+ print_msg(_("PITR shutdown\n"));
+ print_msg(_("update configuration for startup
again if needed\n"));
+ break;
I'm not sure I agree that this is a substantially friendlier message. From
a quick skim of the thread, it seems like you want to avoid sending a scary
error message if Postgres was intentionally shut down while in recovery.
If I got this particular message, I think I would be worried that something
went wrong during my point-in-time restore, and I'd be scrambling to figure
out what configuration this message wants me to update.
If I'm correctly interpreting the intent here, it might be worth fleshing
out the messages a bit more. For example, instead of "PITR shutdown,"
perhaps we could say "shut down while in recovery." And maybe we should
point to the specific settings in the latter message.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Re: make pg_ctl more friendly
Hi, On Wed, 10 Jan 2024 at 06:33, Junwang Zhao wrote: > > Hi Nazir, > > On Tue, Jan 9, 2024 at 9:23 PM Nazir Bilal Yavuz wrote: > > > > v2-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch: > > > > - "Examine the log output.\n"), > > + "Examine the log output\n"), > > progname); > > > > I don't think that this is needed. > There seems to be no common sense for the ending dot when using > write_stderr, so I will leave this not changed. > > > > > Other than that, the patch looks good and I confirm that after PITR > > shutdown: > > > > "PITR shutdown" > > "update configuration for startup again if needed" > > > > message shows up, instead of: > > > > "pg_ctl: could not start server" > > "Examine the log output.". > > > > nitpick: It would be better if the order of the error message cases > > and enums is the same ( i.e. 'POSTMASTER_RECOVERY_SHUTDOWN' before > > 'POSTMASTER_FAILED' in enum ) > Agreed, fixed in V3 Thank you for the update. It looks good to me. -- Regards, Nazir Bilal Yavuz Microsoft
Re: make pg_ctl more friendly
Hi Nazir, On Tue, Jan 9, 2024 at 9:23 PM Nazir Bilal Yavuz wrote: > > Hi, > > Thank you for working on this! I agree that the current message is not > friendly. > > On Thu, 9 Nov 2023 at 10:19, Junwang Zhao wrote: > > > > On Thu, Nov 9, 2023 at 3:08 PM Junwang Zhao wrote: > > > > > > After a PITR shutdown, the db state should be *shut down in recovery*, > > > try the > > > patch attached. > > > > > > > previous patch has some format issues, V2 attached. > > v2-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch: > > - "Examine the log output.\n"), > + "Examine the log output\n"), > progname); > > I don't think that this is needed. There seems to be no common sense for the ending dot when using write_stderr, so I will leave this not changed. > > Other than that, the patch looks good and I confirm that after PITR shutdown: > > "PITR shutdown" > "update configuration for startup again if needed" > > message shows up, instead of: > > "pg_ctl: could not start server" > "Examine the log output.". > > nitpick: It would be better if the order of the error message cases > and enums is the same ( i.e. 'POSTMASTER_RECOVERY_SHUTDOWN' before > 'POSTMASTER_FAILED' in enum ) Agreed, fixed in V3 > > -- > Regards, > Nazir Bilal Yavuz > Microsoft -- Regards Junwang Zhao v3-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch Description: Binary data
Re: make pg_ctl more friendly
Hi, Thank you for working on this! I agree that the current message is not friendly. On Thu, 9 Nov 2023 at 10:19, Junwang Zhao wrote: > > On Thu, Nov 9, 2023 at 3:08 PM Junwang Zhao wrote: > > > > After a PITR shutdown, the db state should be *shut down in recovery*, try > > the > > patch attached. > > > > previous patch has some format issues, V2 attached. v2-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch: - "Examine the log output.\n"), + "Examine the log output\n"), progname); I don't think that this is needed. Other than that, the patch looks good and I confirm that after PITR shutdown: "PITR shutdown" "update configuration for startup again if needed" message shows up, instead of: "pg_ctl: could not start server" "Examine the log output.". nitpick: It would be better if the order of the error message cases and enums is the same ( i.e. 'POSTMASTER_RECOVERY_SHUTDOWN' before 'POSTMASTER_FAILED' in enum ) -- Regards, Nazir Bilal Yavuz Microsoft
Re: make pg_ctl more friendly
Hi,
I thought the PITR shutdown was DB_SHUTDOWN. I made a mistake. The v2
attach looks good.
On Thu, Nov 9, 2023 at 3:19 PM Junwang Zhao wrote:
> On Thu, Nov 9, 2023 at 3:08 PM Junwang Zhao wrote:
> >
> > On Thu, Nov 9, 2023 at 9:57 AM Crisp Lee
> wrote:
> > >
> > > Hi,
> > >
> > > I know it. But my question is not that. I did a PITR operation with
> recovery_target_name and recovery_target_action('shutdown'). The PITR
> process was very short and the PITR was done before pg_ctl check. The
> postmaster shutdown due to recovery_target_action, and there was no crash.
> But pg_ctl told me about startup failure. I think the startup had
> succeeded and the result was not a exception. pg_ctl should tell users
> about detailed messages.
> > >
> > > On Thu, Nov 9, 2023 at 9:32 AM Andres Freund
> wrote:
> > >>
> > >> Hi,
> > >>
> > >> On 2023-11-09 09:29:32 +0800, Crisp Lee wrote:
> > >> > How to judge from 'DB_SHUTDOWNED' that PITR ends normally?
> 'DB_SHUTDOWNED'
> > >> > is just a state, it could not give more meaning, so I reuse the
> > >> > recovery.done.
> > >>
> > >> DB_SHUTDOWNED cannot be encountered while recovery is ongoing. If
> there was a
> > >> hard crash, you'd see DB_IN_ARCHIVE_RECOVERY or such, if the command
> was shut
> > >> down orderly before PITR has finished, you'd see
> DB_SHUTDOWNED_IN_RECOVERY.
> > >>
> > >> - Andres
> >
> > After a PITR shutdown, the db state should be *shut down in recovery*,
> try the
> > patch attached.
> >
>
> previous patch has some format issues, V2 attached.
>
> >
> > --
> > Regards
> > Junwang Zhao
>
>
>
> --
> Regards
> Junwang Zhao
>
Re: make pg_ctl more friendly
On Thu, Nov 9, 2023 at 3:08 PM Junwang Zhao wrote:
>
> On Thu, Nov 9, 2023 at 9:57 AM Crisp Lee wrote:
> >
> > Hi,
> >
> > I know it. But my question is not that. I did a PITR operation with
> > recovery_target_name and recovery_target_action('shutdown'). The PITR
> > process was very short and the PITR was done before pg_ctl check. The
> > postmaster shutdown due to recovery_target_action, and there was no crash.
> > But pg_ctl told me about startup failure. I think the startup had
> > succeeded and the result was not a exception. pg_ctl should tell users
> > about detailed messages.
> >
> > On Thu, Nov 9, 2023 at 9:32 AM Andres Freund wrote:
> >>
> >> Hi,
> >>
> >> On 2023-11-09 09:29:32 +0800, Crisp Lee wrote:
> >> > How to judge from 'DB_SHUTDOWNED' that PITR ends normally?
> >> > 'DB_SHUTDOWNED'
> >> > is just a state, it could not give more meaning, so I reuse the
> >> > recovery.done.
> >>
> >> DB_SHUTDOWNED cannot be encountered while recovery is ongoing. If there
> >> was a
> >> hard crash, you'd see DB_IN_ARCHIVE_RECOVERY or such, if the command was
> >> shut
> >> down orderly before PITR has finished, you'd see DB_SHUTDOWNED_IN_RECOVERY.
> >>
> >> - Andres
>
> After a PITR shutdown, the db state should be *shut down in recovery*, try the
> patch attached.
>
previous patch has some format issues, V2 attached.
>
> --
> Regards
> Junwang Zhao
--
Regards
Junwang Zhao
v2-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch
Description: Binary data
Re: make pg_ctl more friendly
On Thu, Nov 9, 2023 at 9:57 AM Crisp Lee wrote:
>
> Hi,
>
> I know it. But my question is not that. I did a PITR operation with
> recovery_target_name and recovery_target_action('shutdown'). The PITR process
> was very short and the PITR was done before pg_ctl check. The postmaster
> shutdown due to recovery_target_action, and there was no crash. But pg_ctl
> told me about startup failure. I think the startup had succeeded and the
> result was not a exception. pg_ctl should tell users about detailed messages.
>
> On Thu, Nov 9, 2023 at 9:32 AM Andres Freund wrote:
>>
>> Hi,
>>
>> On 2023-11-09 09:29:32 +0800, Crisp Lee wrote:
>> > How to judge from 'DB_SHUTDOWNED' that PITR ends normally? 'DB_SHUTDOWNED'
>> > is just a state, it could not give more meaning, so I reuse the
>> > recovery.done.
>>
>> DB_SHUTDOWNED cannot be encountered while recovery is ongoing. If there was a
>> hard crash, you'd see DB_IN_ARCHIVE_RECOVERY or such, if the command was shut
>> down orderly before PITR has finished, you'd see DB_SHUTDOWNED_IN_RECOVERY.
>>
>> - Andres
After a PITR shutdown, the db state should be *shut down in recovery*, try the
patch attached.
--
Regards
Junwang Zhao
0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch
Description: Binary data
Re: make pg_ctl more friendly
Hi,
I know it. But my question is not that. I did a PITR operation with
recovery_target_name and recovery_target_action('shutdown'). The PITR
process was very short and the PITR was done before pg_ctl check. The
postmaster shutdown due to recovery_target_action, and there was no crash.
But pg_ctl told me about startup failure. I think the startup had
succeeded and the result was not a exception. pg_ctl should tell users
about detailed messages.
On Thu, Nov 9, 2023 at 9:32 AM Andres Freund wrote:
> Hi,
>
> On 2023-11-09 09:29:32 +0800, Crisp Lee wrote:
> > How to judge from 'DB_SHUTDOWNED' that PITR ends normally?
> 'DB_SHUTDOWNED'
> > is just a state, it could not give more meaning, so I reuse the
> > recovery.done.
>
> DB_SHUTDOWNED cannot be encountered while recovery is ongoing. If there
> was a
> hard crash, you'd see DB_IN_ARCHIVE_RECOVERY or such, if the command was
> shut
> down orderly before PITR has finished, you'd see DB_SHUTDOWNED_IN_RECOVERY.
>
> - Andres
>
Re: make pg_ctl more friendly
Hi, On 2023-11-09 09:29:32 +0800, Crisp Lee wrote: > How to judge from 'DB_SHUTDOWNED' that PITR ends normally? 'DB_SHUTDOWNED' > is just a state, it could not give more meaning, so I reuse the > recovery.done. DB_SHUTDOWNED cannot be encountered while recovery is ongoing. If there was a hard crash, you'd see DB_IN_ARCHIVE_RECOVERY or such, if the command was shut down orderly before PITR has finished, you'd see DB_SHUTDOWNED_IN_RECOVERY. - Andres
Re: make pg_ctl more friendly
How to judge from 'DB_SHUTDOWNED' that PITR ends normally? 'DB_SHUTDOWNED' is just a state, it could not give more meaning, so I reuse the recovery.done. On Sat, Nov 4, 2023 at 9:56 AM Andres Freund wrote: > Hi, > > On 2023-11-02 14:50:14 +0800, Crisp Lee wrote: > > I got a basebackup using pg_basebackup -R. After that, I created a > restore > > point named test on primary, and set recovery_target_name to test, > > recovery_target_action to shutdown in standby datadir. I got a failure > > startup message after 'pg_ctl start -D $standby_datadir'. I think it is > > not a failure, and makes users nervous, especially for newbies. > > > > My thought is to generate a recovery.done file if the postmaster receives > > exit code 3 from the startup process. When postmaster exits, pg_ctl will > > give a more friendly message to users. > > I think we can detect this without any additional state - pg_ctl already > accesses pg_control (via get_control_dbstate()). We should be able to > detect > your case by issuing a different warning if > > a) get_control_dbstate() at the start was *not* DB_SHUTDOWNED > b) get_control_dbstate() at the end is DB_SHUTDOWNED > > Greetings, > > Andres Freund >
Re: make pg_ctl more friendly
Hi, On 2023-11-02 14:50:14 +0800, Crisp Lee wrote: > I got a basebackup using pg_basebackup -R. After that, I created a restore > point named test on primary, and set recovery_target_name to test, > recovery_target_action to shutdown in standby datadir. I got a failure > startup message after 'pg_ctl start -D $standby_datadir'. I think it is > not a failure, and makes users nervous, especially for newbies. > > My thought is to generate a recovery.done file if the postmaster receives > exit code 3 from the startup process. When postmaster exits, pg_ctl will > give a more friendly message to users. I think we can detect this without any additional state - pg_ctl already accesses pg_control (via get_control_dbstate()). We should be able to detect your case by issuing a different warning if a) get_control_dbstate() at the start was *not* DB_SHUTDOWNED b) get_control_dbstate() at the end is DB_SHUTDOWNED Greetings, Andres Freund
