Re: pg_basebackup for streaming base backups

On Mon, Jan 17, 2011 at 14:30, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Mon, Jan 17, 2011 at 7:14 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> Oh, sorry about that. There is only one that contains postgresql though :P
>>>
>>> http://github.com/mhagander/postgres, branch streaming_base.
>>
>> Thanks!
>
> Though I haven't seen the core part of the patch (i.e.,
> ReceiveTarFile, etc..) yet,
> here is the comments against others.
>
>
> +               if (strcmp(argv[1], "-h") == 0 || strcmp(argv[1], "--help") == 0 ||
> +                       strcmp(argv[1], "-?") == 0)
>
> strcmp(argv[1], "-h") should be removed

Oh, crap. From the addition of -h for host. oopsie.

> +       printf(_("  -p, --progress            show progress information\n"));
>
> -p needs to be changed to -P

Indeed.

> +       printf(_("  -D, --pgdata=directory   receive base backup into directory\n"));
> +       printf(_("  -T, --tardir=directory    receive base backup into tar files\n"
> +                        "                            stored in specified directory\n"));
> +       printf(_("  -Z, --compress=0-9        compress tar output\n"));
> +       printf(_("  -l, --label=label         set backup label\n"));
> +       printf(_("  -p, --progress            show progress information\n"));
> +       printf(_("  -v, --verbose             output verbose messages\n"));
>
> Can we list those options in alphabetical order as other tools do?

Certainly. But it makes more sense to have -D and -T next to each
other, I think - they'd end up way elsewhere. Perhaps we need a group
taht says "target"?

> Like -f and -F option of pg_dump, it's more intuitive to provide one option for
> output directory and that for format. Something like
>
>    -d directory
>    --dir=directory
>
>    -F format
>    --format=format
>
>    p
>    plain
>
>    t
>    tar

That's another option. It would certainly make for more consistency -
probably a better idea.

> +                       case 'p':
> +                               if (atoi(optarg) == 0)
> +                               {
> +                                       fprintf(stderr, _("%s: invalid port number \"%s\""),
> +                                                       progname, optarg);
> +                                       exit(1);
> +                               }
>
> Shouldn't we emit an error message when the result of atoi is *less than* or
> equal to 0? \n should be in the tail of the error message. Is this error check
> really required here? IIRC libpq does. If it's required, atoi for compresslevel
> should also be error-checked.

Yes on all.

> +                       case 'v':
> +                               verbose++;
>
> Why is the verbose defined as integer?

I envisioned multiple level of verbosity (which I had in
pg_streamrecv), where multiple -v's would add things.

> +       if (optind < argc)
> +       {
> +               fprintf(stderr,
> +                               _("%s: too many command-line arguments (first is \"%s\")\n"),
> +                               progname, argv[optind + 1]);
>
> You need to reference to argv[optind] instead.

Check. Copy/paste:o.

> What about using PGDATA environment variable when no target directory is
> specified?

Hmm. I don't really like that. I prefer requiring it to be specified.

> + * Verify that the given directory exists and is empty. If it does not
> + * exist, it is created. If it exists but is not empty, an error will
> + * be give and the process ended.
> + */
> +static void
> +verify_dir_is_empty_or_create(char *dirname)
>
> Since verify_dir_is_empty_or_create can be called after the connection has
> been established, it should call PQfinish before exit(1).

Yeah, that's a general thing - do we need to actually bother doing
that? At most exit() places I haven't bothered free:ing memory or
closing the connection.

It's not just the PQclear() that you refer to further down - it's also
all the xstrdup()ed strings for example. Do we really need to care
about those before we do exit(1)? I think not.

Requiring PQfinish() might be more reasonable since it will give you a
log on the server if you don't, but I'm not convinced that's necessary
either?

<snip similar requirements>

> +       keywords[2] = "fallback_application_name";
> +       values[2] = "pg_basebackup";
>
> Using the progname variable seems better rather than the fixed word
> "pg_basebackup".

I don't think so - that turns into argv[0], which means that if you
use the full path of the program (/usr/local/pgsql/bin/pg_basebackup
for example), the entire path goes into fallback_application_name -
not just the program name.

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