Thread: 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

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

On Thu, Nov 9, 2023 at 9:57 AM Crisp Lee <litianxiang01@gmail.com> 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
postmastershutdown due to recovery_target_action, and there was no crash. But pg_ctl told me about startup failure.  I
thinkthe 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 <andres@anarazel.de> 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

On Thu, Nov 9, 2023 at 3:08 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> On Thu, Nov 9, 2023 at 9:57 AM Crisp Lee <litianxiang01@gmail.com> 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
postmastershutdown due to recovery_target_action, and there was no crash. But pg_ctl told me about startup failure.  I
thinkthe 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 <andres@anarazel.de> 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

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 <zhjwpku@gmail.com> wrote:
>
> On Thu, Nov 9, 2023 at 3:08 PM Junwang Zhao <zhjwpku@gmail.com> 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

Hi Nazir,

On Tue, Jan 9, 2024 at 9:23 PM Nazir Bilal Yavuz <byavuz81@gmail.com> 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 <zhjwpku@gmail.com> wrote:
> >
> > On Thu, Nov 9, 2023 at 3:08 PM Junwang Zhao <zhjwpku@gmail.com> 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

Hi,

On Wed, 10 Jan 2024 at 06:33, Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> Hi Nazir,
>
> On Tue, Jan 9, 2024 at 9:23 PM Nazir Bilal Yavuz <byavuz81@gmail.com> 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

+    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

Hi Nathan,

On Tue, Jan 16, 2024 at 5:39 AM Nathan Bossart <nathandbossart@gmail.com> 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

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."

Hi Alvaro,

On Wed, Jan 17, 2024 at 4:54 PM Alvaro Herrera <alvherre@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.

>
> 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

On Wed, 2024-01-17 at 17:33 +0800, Junwang Zhao wrote:
> On Wed, Jan 17, 2024 at 4:54 PM Alvaro Herrera <alvherre@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

Hi, Laurenz

On Wed, Jul 10, 2024 at 3:59 AM Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> On Wed, 2024-01-17 at 17:33 +0800, Junwang Zhao wrote:
> > On Wed, Jan 17, 2024 at 4:54 PM Alvaro Herrera <alvherre@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.

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

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

Junwang Zhao <zhjwpku@gmail.com> writes:
> On Wed, Jul 10, 2024 at 3:59 AM Laurenz Albe <laurenz.albe@cybertec.at> 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

Fujii Masao <masao.fujii@oss.nttdata.com> 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

On 2024/07/19 2:58, Tom Lane wrote:
> Fujii Masao <masao.fujii@oss.nttdata.com> 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