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
