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

On 15 November 2013 17:26 Magnus Hagander wrote:
On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi <haribabu(dot)kommi(at)huawei(dot)com<mailto:haribabu(dot)kommi(at)huawei(dot)com>> wrote:
On 14 November 2013 23:59 Fujii Masao wrote:
> On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
> <haribabu(dot)kommi(at)huawei(dot)com<mailto: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.
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.