Re: Improve shutdown during online backup, take 4

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Magnus Hagander <magnus(at)hagander(dot)net>
Cc: Alvaro Herrera <alvherre(at)commandprompt(dot)com>, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>, pgsql-patches(at)postgresql(dot)org
Subject: Re: Improve shutdown during online backup, take 4
Date: 2008-04-23 15:59:11
Message-ID: 12407.1208966351@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-patches

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Alvaro Herrera wrote:
>> I think the messages should not have a newline in the middle.

> Are you talking about the one in pg_ctl? We have other messages in
> pg_ctl that already do this, so I figured it was ok there...

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

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.

regards, tom lane

In response to

Responses

Browse pgsql-patches by date

  From Date Subject
Next Message Joshua D. Drake 2008-04-23 21:41:20 Patch to change psql default banner
Previous Message Alvaro Herrera 2008-04-23 15:16:45 Re: Snapshot management, final