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

On Wed, Nov 20, 2013 at 7:43 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:
> On 19 November 2013 19:12 Fujii Masao wrote:
>> On Tue, Nov 19, 2013 at 9:14 PM, Haribabu kommi
>> <haribabu.kommi@huawei.com> wrote:
>> > 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:
>> >>
>> >> 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.
>
> I tried using of stat'ing in two directories, which is having a problem in windows.
> So modified old approach to detect limited errors. Updated patch attached.
> This will detect and throw an error in the following scenarios.
> 1. pg_basebackup -D /home/data --xlogdir=/home/data
> 2. pg_basebackup -D data --xlogdir=/home/data -- home is the CWD
> 3. pg_basebackup -D ../data --xlogdir=/data -- home is the CWD
>
> Please let me know your suggestions.

Checking only #1 and #2 cases looks sufficient to at least me. If we do that,
we can refactor the patch so that it reuses make_absolute_path() instead of
adding new functions, as pointed in upthread. Anyway, what about committing
the core patch (Updated version of core patch attached) first? Then, we can
discuss the check logic more.

Regards,

--
Fujii Masao