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