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

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

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 <andres@anarazel.de> 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 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 <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 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 <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,

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 <zhjwpku@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.

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