From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | "Albe Laurenz" <laurenz(dot)albe(at)wien(dot)gv(dot)at> |
Cc: | "Simon Riggs <simon(at)2ndquadrant(dot)com>, <pgsql-patches(at)postgresql(dot)org>"(at)hagander(dot)net |
Subject: | Re: Improve shutdown during online backup, take 3 |
Date: | 2008-04-22 13:15:09 |
Message-ID: | 20080422151509.04951b03@mha-laptop |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-patches |
Albe Laurenz wrote:
> Simon Riggs wrote:
> > Patch applies, and works as described. Looks good for final apply.
> >
> > Few minor thoughts:
> >
> > * Text in pg_ctl should be WARNING, not Warning.
> > * CancelBackup() API looks strange, not sure why
> > * Need to mention that CancelBackup() is not the right way to end a
> > backup, so that function and pg_stop_backup should reference
> > each other
> >
> > Other than those, I like it. Very useful patch.
>
> Thanks for the feedback!
>
> - I have replaced "Warning" with WARNING".
> - I have changed the API of CancelBackup() to return void.
> I don't use the return code anyway, and I guess it's less confusing
> and "strange" that way.
> - I have added comments to disambiguate pg_stop_backup() and
> CancelBackup().
>
> CancelBackup now writes a message to the server log if it cannot
> delete backup_label - I hope that's not too verbose...
This doesn't look like our normal coding standards, and should probably
be changed:
+ if (0 != stat(BACKUP_LABEL_FILE, &stat_buf))
(there's a number of similar places)
//Magnus
From | Date | Subject | |
---|---|---|---|
Next Message | Zoltan Boszormenyi | 2008-04-22 15:19:11 | Re: TRUNCATE TABLE with IDENTITY |
Previous Message | Alvaro Herrera | 2008-04-22 12:50:52 | Re: Removing NONSEG mode |