Re: New option for pg_basebackup, to specify a different directory for pg_xlog - Mailing list pgsql-hackers

On 18 November 2013 23:30 Fujii Masao wrote:
> On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
> <haribabu.kommi@huawei.com> wrote:
> > On 18 November 2013 18:45 Fujii Masao wrote:
> >> On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
> >> <haribabu.kommi@huawei.com> wrote:
> >> >
> >> > On 18 November 2013 11:19 Haribabu kommi wrote:
> >> >> On 17 November 2013 00:55 Fujii Masao wrote:
> >> >> > On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
> >> >> > <haribabu.kommi@huawei.com> wrote:
> >> >> > > on 15 November 2013 17:26 Magnus Hagander wrote:
> >> >> > >
> >> >> > >>On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
> >> >> > >><haribabu.kommi@huawei.com> wrote:
> >> >> > >
> >> >> > >>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:
> >> >> > >>>> 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.
> >> >> > >
> >> >> > >
> >> >> > >
> >> >> > > Presently with initdb also user can specify both data and
> xlog
> >> >> > > directories as same.
> >> >> > >
> >> >> > > To prevent the data directory and xlog directory as same,
> >> >> > > there is
> >> >> a
> >> >> > > way in windows (_fullpath api) to get absolute path from a
> >> >> > > relative path, but I didn't get any such possibilities in
> linux.
> >> >> > >
> >> >> > > I didn't find any other way to check it, if anyone have any
> >> >> > > idea regarding this please let me know.
> >> >> >
> >> >> > What about make_absolute_path() in miscinit.c?
> >> >>
> >> >> The make_absoulte_patch() function gets the current working
> >> directory
> >> >> and adds The relative path to CWD, this is not giving proper
> >> absolute
> >> >> path.
> >> >>
> >> >> I have added a new function verify_data_and_xlog_dir_same() which
> >> >> will change the Current working directory to data directory and
> >> >> gets the CWD and the same way for transaction log directory.
> >> >> Compare the both data and xlog directories and throw an error.
> >> >> Please check it
> >> once.
> >> >>
> >> >> Is there any other way to identify that both data and xlog
> >> >> directories are pointing to the same Instead of comparing both
> >> absolute paths?
> >> >>
> >> >> Updated patch attached in the mail.
> >> >
> >> > Failure when the xlogdir doesn't exist is fixed in the updated
> >> > patch
> >> attached in the mail.
> >>
> >> Thanks for updating the patch!
> >>
> >> With the patch, when I specify the same directory in both -D and --
> >> xlogdir, I got the following error.
> >>
> >> $ cd /tmp
> >> $ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
> >> pg_basebackup: could not change directory to "hoge": No such file or
> >> directory
> >
> > Thanks. After finding the xlog directory absolute path returning back
> > to current working Directory is missed, because of this reason it is
> > failing while finding the absolute Path for data directory. Apart
> from this change the code is reorganized a bit.
> >
> > Updated patch is attached in the mail. Please review it once.
>
> Thanks for newer version of the patch!
>
> I found that the empty base directory is created and remains even when
> the same directory is specified in both -D and --xlogdir and the error
> occurs.
> I think that it's
> better to throw an error in that case before creating any new directory.
> Thought?

By creating the base directory only the patch finds whether provided base and
Xlog directories are same or not? To solve this problem following options are possible

1. No problem as it is just an empty base directory, so it can be reused in the next time  Leave it as it is.
2. Once the error is identified, the base directory can be deleted.
3. write a new function to remove the parent references from the provided path to identify  the absolute path used for
detectingbase and Xlog directories are same or not?  

Please provide your suggestions.

> +    xlogdir = get_absolute_path(xlog_dir);
>
> xlog_dir must be an absolute path. ISTM we don't do the above. No?

It is required. As user can provide the path as /home/installation/bin/../bin/data.
The provided path is considered as absolute path only but while comparing the same
With data directory path it will not match.

Regards,
Hari babu.