Re: New option for pg_basebackup, to specify a different directory for pg_xlog

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:
> Please find attached the patch, for adding a new option for pg_basebackup,
> to specify a different directory for pg_xlog.

Sounds good! Here are the review comments:

+    printf(_("    --xlogdir=XLOGDIR       location for the
transaction log directory\n"));

This message is not aligned well.

-                        if (!streamwal || strcmp(filename +
strlen(filename) - 8, "/pg_xlog") != 0)
+                        if ((!streamwal && (strcmp(xlog_dir, "") == 0))
+                            || strcmp(filename + strlen(filename) -
8, "/pg_xlog") != 0)

You need to update the source code comment.

+#ifdef HAVE_SYMLINK
+        if (symlink(xlog_dir, linkloc) != 0)
+        {
+            fprintf(stderr, _("%s: could not create symbolic link
\"%s\": %s\n"),
+                    progname, linkloc, strerror(errno));
+            exit(1);
+        }
+#else
+        fprintf(stderr, _("%s: symlinks are not supported on this platform "
+                          "cannot use xlogdir"));
+        exit(1);
+#endif
+    }

Is it better to call pg_free() at the end? Even if we don't do that, it would be
almost harmless, though.

Don't we need to prevent users from specifying the same directory in both
--pgdata and --xlogdir?

Regards,

-- 
Fujii Masao