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

On Tue, Nov 19, 2013 at 9:14 PM, Haribabu kommi
<haribabu(dot)kommi(at)huawei(dot)com> wrote:
> On 18 November 2013 23:30 Fujii Masao wrote:
>> On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
>> <haribabu(dot)kommi(at)huawei(dot)com> wrote:
>> > On 18 November 2013 18:45 Fujii Masao wrote:
>> >> On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
>> >> <haribabu(dot)kommi(at)huawei(dot)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(dot)kommi(at)huawei(dot)com> wrote:
>> >> >> > > 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> 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> 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 detecting base 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.

Okay, maybe I understand you. In order to know the real absolute path, basically
we need to create the directory specified in --xlogdir, change the
working directory
to it and calculate the parent path. You're worried about the cases as
follows, for example.
Right?

* path containing ".." like /aaa/bbb/../ccc is specified in --xlogdir
* symbolic link is specified in --xlogdir

On the second thought, I'm thinking that it might be overkill to add
such not simple code for that small benefit.

Regards,

--
Fujii Masao