Re: pg_basebackup for streaming base backups

On Wed, Jan 19, 2011 at 06:14, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, Jan 19, 2011 at 4:12 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> Ah, ok. I've added the errorcode now, PFA. I also fixed an error in
>> the change for result codes I broke in the last patch. github branch
>> updated as usual.
>
> Great. Thanks for the quick update!
>
> Here are another comments:
>
> + * IDENTIFICATION
> + *               src/bin/pg_basebackup.c
>
> Typo: s/"src/bin/pg_basebackup.c"/"src/bin/pg_basebackup/pg_basebackup.c"

Oops.

> +       printf(_("  -c, --checkpoint=fast|slow\n"
> +                        "                            set fast or slow checkpoinging\n"));
>
> Typo: s/checkpoinging/checkpointing
>
> The "fast or slow" seems to lead users to always choose "fast". Instead,
> what about "fast or smooth", "fast or spread" or "immediate or delayed"?

Hmm. "fast or spread" seems reasonable to me. And I want to use "fast"
for the fast version, because that's what we call it when you use
pg_start_backup(). I'll go change it to spread for now - it's the one
I can find used in the docs.

> You seem to have forgotten to support "--checkpoint" long option.
> The struct long_options needs to be updated.

Wow, that clearly went too fast. Fixed as wlel.

> What if pg_basebackup receives a signal while doing a backup?
> For example, users might do Ctrl-C to cancel the long-running backup.
> We should define a signal handler and send a Terminate message
> to the server to cancel the backup?

Nah, we'll just disconnect and it'll deal with things that way. Just
like we do with e.g. pg_dump. I don't see the need to complicate it
with that.

(new patch on github in 5 minutes)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/