Re: Improve shutdown during online backup, take 4

Tom Lane wrote:
> I concur that the messages added to pg_ctl are bizarrely formatted.
> Why would you put a newline in the middle of a sentence, when you
> could equally well emit something like
>
> WARNING: online backup mode is active.
> Shutdown will not complete until pg_stop_backup() is called.
>
> While we're on the subject, the messages added to xlog.c do not
> follow the style guidelines: in particular, errdetail should be
> a complete sentence, and the WARNING is trying to stuff independent
> thoughts into one message. I'd probably do
>
> errmsg("online backup mode cancelled"),
> errdetail("\"%s\" was renamed to \"%s\".", ...
>
> errmsg("online backup mode was not cancelled"),
> errdetail("Failed to rename \"%s\" to \"%s\": %m", ...

Attached is a patch that changes the messages along these lines.
Thanks!

> Lastly, the changes to pmdie's SIGINT handling seem quite bogus.
> Don't you need to transition into WAIT_BACKUP rather than WAIT_BACKENDS
> state in that case too? Shouldn't you do CancelBackup *before*
> PostmasterStateMachine? The thing screams of race conditions.

I suspect there must be a misunderstanding.
You cannot really mean that the postmaster should enter WAIT_BACKUP
state on a fast shutdown request.

Sure, CancelBackup could be called earlier. It doesn't do much more than
rename a file.
My reason for calling it late was that backup mode should really only be
cancelled if we manage to shutdown cleanly, and this is not clear until
the last child is gone.

I cannot see a race condition, except maybe the quite unlikely case
that two instances of CancelBackup might run concurrently, and it
*might* happen that one of them finds that "backup_label" is present but
fails to remove it, because the other one already did.
That would lead to a bogus "backup mode not canceled" message.

Are you referring to that or is there something more fundamental
I fail to see?

Yours,
Laurenz Albe