Hi Nathan,

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

Attachment: v4-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch
Description: Binary data

Reply via email to