Re: pg_basebackup for streaming base backups

On Mon, Jan 17, 2011 at 10:30 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> Though I haven't seen the core part of the patch (i.e.,
> ReceiveTarFile, etc..) yet,
> here is the comments against others.

Here are another comments:

When I untar the tar file taken by pg_basebackup, I got the following
messages:

$ tar xf base.tar
tar: Skipping to next header
tar: Archive contains obsolescent base-64 headers
tar: Error exit delayed from previous errors

Is this a bug? This happens only when I create $PGDATA by using
initdb -X (i.e., I relocated the pg_xlog directory elsewhere than
$PGDATA).

+ if (compresslevel > 0)
+ {
+ snprintf(fn, sizeof(fn), "%s/%s.tar.gz", tardir, PQgetvalue(res,
rownum, 0));
+ ztarfile = gzopen(fn, "wb");

Though I'm not familiar with zlib, isn't gzsetparams() required
here?

+#ifdef HAVE_LIBZ
+ if (!tarfile && !ztarfile)
+#else
+ if (!tarfile)
+#endif
+ {
+ fprintf(stderr, _("%s: could not create file \"%s\": %s\n"),
+ progname, fn, strerror(errno));

Instead of strerror, get_gz_error seems required when using zlib.

+ if (!res || PQresultStatus(res) != PGRES_COPY_OUT)

The check for "!res" is not needed since PQresultStatus checks that.

+ r = PQgetCopyData(conn, &copybuf, 0);
+ if (r == -1)

Since -1 of PQgetCopyData might indicate an error, in this case,
we would need to call PQgetResult?.

ReceiveTarFile seems refactorable by using GZWRITE and GZCLOSE
macros.

+ fprintf(stderr, _("%s: could not write to file '%s': %m\n"),

%m in fprintf is portable?

Can't you change '%s' to \"%s\" for consistency?

+ /*
+ * Make sure we're unpacking into an empty directory
+ */
+ verify_dir_is_empty_or_create(current_path);

Can pg_basebackup take a backup of $PGDATA including a tablespace
directory, without an error? The above code seems to prevent that....

+ if (compresslevel <= 0)
+ {
+ fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),

It's better to check "compresslevel > 9" here.

+/*
+ * Print a progress report based on the global variables. If verbose output
+ * is disabled, also print the current file name.

Typo: s/disabled/enabled

I request new option which specifies whether pg_start_backup
executes immediate checkpoint or not. Currently it always executes
immediate one. But I'd like to run smoothed one in busy system.
What's your opinion?

Regards,

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