Re: pg_basebackup for streaming base backups - Mailing list pgsql-hackers

On Mon, Jan 17, 2011 at 10:30 PM, Fujii Masao <masao.fujii@gmail.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
exitdelayed 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, ©buf, 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