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

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.