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(dot)kommi(at)huawei(dot)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