Re: pg_basebackup for streaming base backups

On Mon, Jan 17, 2011 at 10:50 PM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>> +       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"?

I agree with you if we end up choosing -D and -T for a target rather
than pg_dump-like options I proposed.

>> + * 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.

Probably true. The allocated memory would be free'd right after
exit.

> 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?

At least it's required for each password-retry. Otherwise, previous
connections remain during backup. You can see this problem easily
by repeating the password input in pg_basebackup.

LOG: could not send data to client: Connection reset by peer
LOG: could not send data to client: Broken pipe
FATAL: base backup could not send data, aborting backup

As you said, if PQfinish is not called at exit(1), the above messages
would be output. Those messages look ugly and should be
suppressed whenever we *can*. Also I believe other tools would
do that.

>> +       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.

No. get_progname extracts the actual name.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center