On Tue, Nov 19, 2013 at 2:41 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
> On Tue, Nov 19, 2013 at 9:14 PM, Haribabu kommi > <haribabu.ko...@huawei.com> wrote: > > On 18 November 2013 23:30 Fujii Masao wrote: > >> On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi > >> <haribabu.ko...@huawei.com> wrote: > >> > On 18 November 2013 18:45 Fujii Masao wrote: > >> >> On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi > >> >> <haribabu.ko...@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.ko...@huawei.com> wrote: > >> >> >> > > on 15 November 2013 17:26 Magnus Hagander wrote: > >> >> >> > > > >> >> >> > >>On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi > >> >> >> > >><haribabu.ko...@huawei.com> wrote: > >> >> >> > > > >> >> >> > >>On 14 November 2013 23:59 Fujii Masao wrote: > >> >> >> > >>> On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi > >> >> >> > >>> <haribabu.ko...@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 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. > What I actually was *looking* for when I said I thought we should have that check, was just to find *manual* errors. As in, I wanted to cover the case where the user said "-D /my/backup --xlogdir /my/backup", thinking that it had to be specified. If they end up that way through a series of symlinks or something like that, I think we can just punt that as user error and they'll find out on their own later, we don't need to catch it and throw an error. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/