Re: make pg_ctl more friendly - Mailing list pgsql-hackers

From Junwang Zhao
Subject Re: make pg_ctl more friendly
Date
Msg-id CAEG8a3Ldgco=1DHDbOmyhNa_ewsKTy9TAD7HuxPdWXkY2ZsUiA@mail.gmail.com
Whole thread Raw
In response to Re: make pg_ctl more friendly  (Laurenz Albe <laurenz.albe@cybertec.at>)
Responses Re: make pg_ctl more friendly
Re: make pg_ctl more friendly
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: pgsql: Add pg_get_acl() to get the ACL for a database object
Next
From: Masahiko Sawada
Date:
Subject: Re: MERGE/SPLIT partition commands should create new partitions in the parent's tablespace?