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
make pg_ctl more friendly
Hi hackers: 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. 0001-Make-pg_ctl-more-friendly-for-users.patch Description: Binary data
