On Mon, Jan 17, 2011 at 14:30, Fujii Masao <masao.fu...@gmail.com> wrote:
> On Mon, Jan 17, 2011 at 7:14 PM, Fujii Masao <masao.fu...@gmail.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/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
