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
