From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
Cc: | Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: pg_basebackup for streaming base backups |
Date: | 2011-01-17 13:50:16 |
Message-ID: | AANLkTi=R3LgGuqziPyYabvzqdn6RMWwcLUN68JYHzki3@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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/
From | Date | Subject | |
---|---|---|---|
Next Message | Magnus Hagander | 2011-01-17 13:54:50 | Re: pg_basebackup for streaming base backups |
Previous Message | Simon Riggs | 2011-01-17 13:49:56 | Re: pg_basebackup for streaming base backups |