On 14 November 2013 23:59 Fujii Masao wrote: > 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.
Fixed.
> - 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.
Corrected the source code comment. Please check once.
> +#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.
Added pg_free to free up the linkloc.
> Don't we need to prevent users from specifying the same directory in > both --pgdata and --xlogdir?
I feel no need to prevent, even if user specifies both --pgdata and --xlogdir as same directory all the transaction log files will be created in the base directory instead of pg_xlog directory.
Given how easy it would be to prevent that, I think we should. It would be an easy misunderstanding to specify that when you actually want it in <wherever>/pg_xlog. Specifying that would be redundant in the first place, but people ca do that, but it would also be very easy to do it by mistake, and you'd end up with something that's really bad, including a recursive symlink.
I also think it would probably be worthwhile to support this in tar format as well, but I guess that's a separate patch really. There's really no reason we should't support wal streaming into a separate tar file. But - separate issue.
