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.
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"
--
Á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."
