Hi Alvaro,
On Wed, Jan 17, 2024 at 4:54 PM Alvaro Herrera <alvhe...@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
v5-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch
Description: Binary data
