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

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?

+    xlogdir = get_absolute_path(xlog_dir);

xlog_dir must be an absolute path. ISTM we don't do the above. No?

Regards,

-- 
Fujii Masao