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

Please find attached the patch, for adding a new option for pg_basebackup, to specify a different directory for pg_xlog.

Design

A new option: "xlogdir" is added to the list of options for pg_basebackup. The new option is not having an equivalent short option letter.
This option will allow the user to specify a different directory for pg_xlog.

The format for specifying a different directory will be: --xlogdir=/path/to/xlog/directory

eg:
pg_basebackup --xlogdir=/home/pg/xlog -D ../dataBaseBackUp

When user specifies a xlog directory, it creates a symbolic link from the default directory to the user specified directory.
User can give only absolute path for the xlog directory. This option will work only if the format for the backup is 'plain'.

Please provide your feedback / suggestions

Regards,
Hari babu.

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:

+ printf(_(" --xlogdir=XLOGDIR location for the
transaction log directory\n"));

This message is not aligned well.

- if (!streamwal || strcmp(filename +
strlen(filename) - 8, "/pg_xlog") != 0)
+ if ((!streamwal && (strcmp(xlog_dir, "") == 0))
+ || strcmp(filename + strlen(filename) -
8, "/pg_xlog") != 0)

You need to update the source code comment.

+#ifdef HAVE_SYMLINK
+ if (symlink(xlog_dir, linkloc) != 0)
+ {
+ fprintf(stderr, _("%s: could not create symbolic link
\"%s\": %s\n"),
+ progname, linkloc, strerror(errno));
+ exit(1);
+ }
+#else
+ fprintf(stderr, _("%s: symlinks are not supported on this platform "
+ "cannot use xlogdir"));
+ exit(1);
+#endif
+ }

Is it better to call pg_free() at the end? Even if we don't do that, it would be
almost harmless, though.

Don't we need to prevent users from specifying the same directory in both
--pgdata and --xlogdir?

Regards,

--
Fujii Masao

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:
>
> + printf(_(" --xlogdir=XLOGDIR location for the
> transaction log directory\n"));
>
> This message is not aligned well.

Fixed.

> - if (!streamwal || strcmp(filename +
> strlen(filename) - 8, "/pg_xlog") != 0)
> + if ((!streamwal && (strcmp(xlog_dir, "") == 0))
> + || strcmp(filename + strlen(filename) -
> 8, "/pg_xlog") != 0)
>
> You need to update the source code comment.

Corrected the source code comment. Please check once.

> +#ifdef HAVE_SYMLINK
> + if (symlink(xlog_dir, linkloc) != 0)
> + {
> + fprintf(stderr, _("%s: could not create symbolic link
> \"%s\": %s\n"),
> + progname, linkloc, strerror(errno));
> + exit(1);
> + }
> +#else
> + fprintf(stderr, _("%s: symlinks are not supported on this
> platform "
> + "cannot use xlogdir"));
> + exit(1);
> +#endif
> + }
>
> Is it better to call pg_free() at the end? Even if we don't do that, it
> would be almost harmless, though.

Added pg_free to free up the linkloc.

> 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.

Regards,
Hari babu.

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:
> >
> > + printf(_(" --xlogdir=XLOGDIR location for the
> > transaction log directory\n"));
> >
> > This message is not aligned well.
>
> Fixed.
>
> > - if (!streamwal || strcmp(filename +
> > strlen(filename) - 8, "/pg_xlog") != 0)
> > + if ((!streamwal && (strcmp(xlog_dir, "") == 0))
> > + || strcmp(filename + strlen(filename) -
> > 8, "/pg_xlog") != 0)
> >
> > You need to update the source code comment.
>
> Corrected the source code comment. Please check once.
>
> > +#ifdef HAVE_SYMLINK
> > + if (symlink(xlog_dir, linkloc) != 0)
> > + {
> > + fprintf(stderr, _("%s: could not create symbolic link
> > \"%s\": %s\n"),
> > + progname, linkloc, strerror(errno));
> > + exit(1);
> > + }
> > +#else
> > + fprintf(stderr, _("%s: symlinks are not supported on this
> > platform "
> > + "cannot use xlogdir"));
> > + exit(1);
> > +#endif
> > + }
> >
> > Is it better to call pg_free() at the end? Even if we don't do that, it
> > would be almost harmless, though.
>
> Added pg_free to free up the linkloc.
>
> > 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.

I also think it would probably be worthwhile to support this in tar format
as well, but I guess that's a separate patch really. There's really no
reason we should't support wal streaming into a separate tar file. But -
separate issue.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

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<mailto: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<mailto: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:
>
> + printf(_(" --xlogdir=XLOGDIR location for the
> transaction log directory\n"));
>
> This message is not aligned well.
Fixed.

> - if (!streamwal || strcmp(filename +
> strlen(filename) - 8, "/pg_xlog") != 0)
> + if ((!streamwal && (strcmp(xlog_dir, "") == 0))
> + || strcmp(filename + strlen(filename) -
> 8, "/pg_xlog") != 0)
>
> You need to update the source code comment.
Corrected the source code comment. Please check once.

> +#ifdef HAVE_SYMLINK
> + if (symlink(xlog_dir, linkloc) != 0)
> + {
> + fprintf(stderr, _("%s: could not create symbolic link
> \"%s\": %s\n"),
> + progname, linkloc, strerror(errno));
> + exit(1);
> + }
> +#else
> + fprintf(stderr, _("%s: symlinks are not supported on this
> platform "
> + "cannot use xlogdir"));
> + exit(1);
> +#endif
> + }
>
> Is it better to call pg_free() at the end? Even if we don't do that, it
> would be almost harmless, though.
Added pg_free to free up the linkloc.

> 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.

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<mailto: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<mailto: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.

>I also think it would probably be worthwhile to support this in tar format as well, but I guess that's a separate patch really. There's really no reason we should't support wal streaming into a separate tar file. But - separate issue.

Sure. I will prepare a separate patch for the same and submit it in the next commit fest.

Regards,
Hari babu.

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?

Regards,

--
Fujii Masao

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.

Regards,
Hari babu.

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.

Regards,
Hari babu.

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

Regards,

--
Fujii Masao

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.

Regards,
Hari babu.

On 2013-11-18 15:01:42 +0000, Haribabu kommi wrote:
>
> /*
> + * Returns the malloced string of containing current working directory.
> + * The caller has to take care of freeing the memory.
> + * On failure exits with error code.
> + */
> +static char *
> +get_current_working_dir()
> +{
> + char *buf;
> + size_t buflen;
> +
> + buflen = MAXPGPATH;
> + for (;;)
> + {
> + buf = pg_malloc(buflen);
> + if (getcwd(buf, buflen))
> + break;
> + else if (errno == ERANGE)
> + {
> + pg_free(buf);
> + buflen *= 2;
> + continue;
> + }
> + else
> + {
> + pg_free(buf);
> + fprintf(stderr,
> + _("%s: could not get current working directory: %s\n"),
> + progname, strerror(errno));
> + exit(1);
> + }
> + }
> +
> + return buf;
> +}
> +
> +/*
> + * calculates the absolute path for the given path. The output absolute path
> + * is a malloced string. The caller needs to take care of freeing the memory.
> + */
> +static char *
> +get_absolute_path(const char *input_path)
> +{
> + char *pwd = NULL;
> + char *abspath = NULL;
> +
> + /* Getting the present working directory */
> + pwd = get_current_working_dir();
> +
> + if (chdir(input_path) < 0)
> + {
> + /* Directory doesn't exist */
> + if (errno == ENOENT)
> + return NULL;
> +
> + fprintf(stderr, _("%s: could not change directory to \"%s\": %s\n"),
> + progname, input_path, strerror(errno));
> + exit(1);
> + }
> +
> + abspath = get_current_working_dir();
> +
> + /* Returning back to old working directory */
> + if (chdir(pwd) < 0)
> + {
> + fprintf(stderr, _("%s: could not change directory to \"%s\": %s\n"),
> + progname, pwd, strerror(errno));
> + exit(1);
> + }
> +
> + pg_free(pwd);
> + return abspath;
> +}

This looks like it should be replaced by moving make_absolute_path from
pg_regress.c to path.[ch] and then use that.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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?

+ xlogdir = get_absolute_path(xlog_dir);

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

Regards,

--
Fujii Masao

On 18 November 2013 23:25 Andres Freund wrote:
> On 2013-11-18 15:01:42 +0000, Haribabu kommi wrote:
> >
> > /*
> > + * Returns the malloced string of containing current working
> directory.
> > + * The caller has to take care of freeing the memory.
> > + * On failure exits with error code.
> > + */
> > +static char *
> > +get_current_working_dir()
> > +{
> > + char *buf;
> > + size_t buflen;
> > +
> > + buflen = MAXPGPATH;
> > + for (;;)
> > + {
> > + buf = pg_malloc(buflen);
> > + if (getcwd(buf, buflen))
> > + break;
> > + else if (errno == ERANGE)
> > + {
> > + pg_free(buf);
> > + buflen *= 2;
> > + continue;
> > + }
> > + else
> > + {
> > + pg_free(buf);
> > + fprintf(stderr,
> > + _("%s: could not get current working
> directory: %s\n"),
> > + progname, strerror(errno));
> > + exit(1);
> > + }
> > + }
> > +
> > + return buf;
> > +}
> > +
> > +/*
> > + * calculates the absolute path for the given path. The output
> > +absolute path
> > + * is a malloced string. The caller needs to take care of freeing
> the memory.
> > + */
> > +static char *
> > +get_absolute_path(const char *input_path) {
> > + char *pwd = NULL;
> > + char *abspath = NULL;
> > +
> > + /* Getting the present working directory */
> > + pwd = get_current_working_dir();
> > +
> > + if (chdir(input_path) < 0)
> > + {
> > + /* Directory doesn't exist */
> > + if (errno == ENOENT)
> > + return NULL;
> > +
> > + fprintf(stderr, _("%s: could not change directory to
> \"%s\": %s\n"),
> > + progname, input_path, strerror(errno));
> > + exit(1);
> > + }
> > +
> > + abspath = get_current_working_dir();
> > +
> > + /* Returning back to old working directory */
> > + if (chdir(pwd) < 0)
> > + {
> > + fprintf(stderr, _("%s: could not change directory to
> \"%s\": %s\n"),
> > + progname, pwd, strerror(errno));
> > + exit(1);
> > + }
> > +
> > + pg_free(pwd);
> > + return abspath;
> > +}
>
> This looks like it should be replaced by moving make_absolute_path from
> pg_regress.c to path.[ch] and then use that.

The "get_absolute_path" function removes any parent references, if exists, in the path
But the "make_absolute_path" doesn't. In this patch proper path is required to compare
The xlog and data directories provided are same or not?

I find two ways to do this
1. change to that provided directory and get the current working directory.
2. Write a function to remove the parent references in the path.

This patch has implemented the first approach. Please let me know your suggestions.

Regards,
Hari babu.

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.

Regards,
Hari babu.

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

Fujii Masao <masao(dot)fujii(at)gmail(dot)com> writes:
> On the second thought, I'm thinking that it might be overkill to add
> such not simple code for that small benefit.

Yeah --- there's a limit to how much code we should expend on detecting
this type of error. It's not like the case is all that plausible.

One idea that I don't think got discussed is stat'ing the two directories
and verifying that their dev/inode numbers are different. I don't know
how portable that is though.

regards, tom lane

On Tue, Nov 19, 2013 at 2:41 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> 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.
>

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/

On 19 November 2013 19:12 Fujii Masao wrote:
> 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:
> >>
> >> 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.

Regards,
Hari babu.

On 20/11/13 23:43, Haribabu kommi wrote:
> On 19 November 2013 19:12 Fujii Masao wrote:
>> 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:
>>>>
>>>> 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.
>
> Regards,
> Hari babu.
>
>
I don't think Postgres on other systems should be hobbled by the
limitations of Microsoft software!

If certain features of Postgres are either not available, or are
available in a reduced form on Microsoft platforms, then this should be
documented - might provide subtle hints to upgrade to Linux.

Cheers,
Gavin

on 20 November 2013 23:44 Gavin Flower wrote:
>On 20/11/13 23:43, Haribabu kommi wrote:

>>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

>I don't think Postgres on other systems should be hobbled by the limitations of Microsoft software!
>If certain features of Postgres are either not available, or are available in a reduced form on Microsoft platforms, then this should be documented - might provide subtle hints to upgrade to Linux.

The patch which sent in earlier mail provides the detection of base and xlog directories are same or not
In both windows and linux with a different way of identifying the same without stat'ing.
If other also agrees to add stat'ing then I will change the patch accordingly and document the behavior
Change for windows.

Regards,
Hari babu.

On Wed, Nov 20, 2013 at 7:43 PM, Haribabu kommi
<haribabu(dot)kommi(at)huawei(dot)com> wrote:
> On 19 November 2013 19:12 Fujii Masao wrote:
>> 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:
>> >>
>> >> 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

On 26 November 2013 23:11 Fujii Masao wrote:
> On Wed, Nov 20, 2013 at 7:43 PM, Haribabu kommi
> <haribabu(dot)kommi(at)huawei(dot)com> wrote:
> > 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.

Yes it is fine. The core patch attached in the mail is working fine.

I will provide another patch with check logic by incorporating your suggestions.

Regards,
Hari babu.

On Wed, Nov 27, 2013 at 1:27 PM, Haribabu kommi
<haribabu(dot)kommi(at)huawei(dot)com> wrote:
> On 26 November 2013 23:11 Fujii Masao wrote:
>> On Wed, Nov 20, 2013 at 7:43 PM, Haribabu kommi
>> <haribabu(dot)kommi(at)huawei(dot)com> wrote:
>> > 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.
>
> Yes it is fine. The core patch attached in the mail is working fine.

Okay. Committed!

Regards,

--
Fujii Masao

On 27 November 2013 10:35 Fujii Masao wrote:
> On Wed, Nov 27, 2013 at 1:27 PM, Haribabu kommi
> <haribabu(dot)kommi(at)huawei(dot)com> wrote:
> > On 26 November 2013 23:11 Fujii Masao wrote:
> >> On Wed, Nov 20, 2013 at 7:43 PM, Haribabu kommi
> >> <haribabu(dot)kommi(at)huawei(dot)com> wrote:
> >> > 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.
> >
> > Yes it is fine. The core patch attached in the mail is working fine.
>
> Okay. Committed!

Thanks. Regarding refactoring the code to use make_absolute_path for checking
the data and xlog directories are same or not, this function exists in two places
1. pg_regress.c 2.miscinit.c.

Solution -1: if we move the backend function to path.c, it cannot use the ereport
Functions. So it may cause problem to backends.

Solution -2: Rename the backend function as make_absolute_path_internal and add a
New function to the path.c

Please let me know your opinion on the same or is there anything I missed?

Regards,
Hari babu.

On 27 November 2013 10:35 Fujii Masao wrote:
> On Wed, Nov 27, 2013 at 1:27 PM, Haribabu kommi
> <haribabu(dot)kommi(at)huawei(dot)com> wrote:
> > On 26 November 2013 23:11 Fujii Masao wrote:
> >> On Wed, Nov 20, 2013 at 7:43 PM, Haribabu kommi
> >> <haribabu(dot)kommi(at)huawei(dot)com> wrote:
> >> > 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.
> >
> > Yes it is fine. The core patch attached in the mail is working fine.
>
> Okay. Committed!

To detect provided data and xlog directories are same or not, I reused the
Existing make_absolute_path() code as follows.

1. Moved the make_absolute_path() function from miscinit.c to path.c and
Changed all error reporting functions. And also it returns NULL incase of
any error.

2. Added a new file called fe_path.c which contains make_absolute_path()
only for frontend code.

The patches are attached in the mail for both approaches, please review and let
Me know your suggestions.

On top those patches, the error detection logic is added in pg_basebackup and the
Same is attached in the mail.

Regards,
Hari babu.

Haribabu kommi escribió:

> To detect provided data and xlog directories are same or not, I reused the
> Existing make_absolute_path() code as follows.
>
> 1. Moved the make_absolute_path() function from miscinit.c to path.c and
> Changed all error reporting functions. And also it returns NULL incase of
> any error.
>
> 2. Added a new file called fe_path.c which contains make_absolute_path()
> only for frontend code.

Whatever you do, please don't add #include lines to postgres_fe.h. Add
them to whatever .c files that need to include the new header, instead.
(This results in a longer patch, yes, but that consideration shouldn't
drive anything. There is a desire to include as less headers as
possible in each source file, and adding more include lines to
postgres_fe.h means the new header will be included by every single
frontend file, even those not in core.)

See a nearby patch by Bruce Momjian to deal with getpwnam() and
getpwuid() failures; perhaps the idea of returning an error string
should be designed similarly in both these patches. Also consider using
the psprintf stuff, which works on both backend and frontend, avoiding
malloc etc so that code can be shared by both frontend and backend,
eliminating the duplicity.

Thanks,

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 10 December 2013 19:55 Alvaro Herrera wrote:
> Haribabu kommi escribió:
>
> > To detect provided data and xlog directories are same or not, I
> reused
> > the Existing make_absolute_path() code as follows.
> >
> > 1. Moved the make_absolute_path() function from miscinit.c to path.c
> > and Changed all error reporting functions. And also it returns NULL
> > incase of any error.
> >
> > 2. Added a new file called fe_path.c which contains
> > make_absolute_path() only for frontend code.
>
> Whatever you do, please don't add #include lines to postgres_fe.h. Add
> them to whatever .c files that need to include the new header, instead.
> (This results in a longer patch, yes, but that consideration shouldn't
> drive anything. There is a desire to include as less headers as
> possible in each source file, and adding more include lines to
> postgres_fe.h means the new header will be included by every single
> frontend file, even those not in core.)
>
> See a nearby patch by Bruce Momjian to deal with getpwnam() and
> getpwuid() failures; perhaps the idea of returning an error string
> should be designed similarly in both these patches. Also consider
> using the psprintf stuff, which works on both backend and frontend,
> avoiding malloc etc so that code can be shared by both frontend and
> backend, eliminating the duplicity.

The make_absolute_path() function moving to port is changed in similar way as
Bruce Momjian approach. The psprintf is used to store the error string which
Occurred in the function. But psprintf is not used for storing the absolute path
As because it is giving problems in freeing the allocated memory in SelectConfigFiles.
Because the same memory is allocated in a different code branch from guc_malloc.

After adding the make_absolute_path() function with psprintf stuff in path.c file
It is giving linking problem in compilation of ecpg. I am not able to find the problem.
So I added another file abspath.c in port which contains these two functions.

Updated patches are attached in the mail. Please provide your suggestions.

Regards,
Hari babu.

On Wed, Dec 11, 2013 at 10:22:32AM +0000, Haribabu kommi wrote:
> The make_absolute_path() function moving to port is changed in similar way as
> Bruce Momjian approach. The psprintf is used to store the error string which
> Occurred in the function. But psprintf is not used for storing the absolute path
> As because it is giving problems in freeing the allocated memory in SelectConfigFiles.
> Because the same memory is allocated in a different code branch from guc_malloc.
>
> After adding the make_absolute_path() function with psprintf stuff in path.c file
> It is giving linking problem in compilation of ecpg. I am not able to find the problem.
> So I added another file abspath.c in port which contains these two functions.

What errors are you seeing?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On 19 December 2013 05:31 Bruce Momjian wrote:
> On Wed, Dec 11, 2013 at 10:22:32AM +0000, Haribabu kommi wrote:
> > The make_absolute_path() function moving to port is changed in
> similar
> > way as Bruce Momjian approach. The psprintf is used to store the
> error
> > string which Occurred in the function. But psprintf is not used for
> > storing the absolute path As because it is giving problems in freeing
> the allocated memory in SelectConfigFiles.
> > Because the same memory is allocated in a different code branch from
> guc_malloc.
> >
> > After adding the make_absolute_path() function with psprintf stuff in
> > path.c file It is giving linking problem in compilation of ecpg. I am
> not able to find the problem.
> > So I added another file abspath.c in port which contains these two
> functions.
>
> What errors are you seeing?

If I move the make_absolute_path function from abspath.c to path.c,
I was getting following linking errors while compiling "ecpg".

../../../../src/port/libpgport.a(path.o): In function `make_absolute_path':
/home/hari/postgres/src/port/path.c:795: undefined reference to `psprintf'
/home/hari/postgres/src/port/path.c:809: undefined reference to `psprintf'
/home/hari/postgres/src/port/path.c:818: undefined reference to `psprintf'
/home/hari/postgres/src/port/path.c:830: undefined reference to `psprintf'
collect2: ld returned 1 exit status
make[4]: *** [ecpg] Error 1
make[3]: *** [all-preproc-recurse] Error 2
make[2]: *** [all-ecpg-recurse] Error 2
make[1]: *** [all-interfaces-recurse] Error 2
make: *** [all-src-recurse] Error 2

Regards,
Hari babu.

On Thu, Dec 19, 2013 at 05:14:50AM +0000, Haribabu kommi wrote:
> On 19 December 2013 05:31 Bruce Momjian wrote:
> > On Wed, Dec 11, 2013 at 10:22:32AM +0000, Haribabu kommi wrote:
> > > The make_absolute_path() function moving to port is changed in
> > similar
> > > way as Bruce Momjian approach. The psprintf is used to store the
> > error
> > > string which Occurred in the function. But psprintf is not used for
> > > storing the absolute path As because it is giving problems in freeing
> > the allocated memory in SelectConfigFiles.
> > > Because the same memory is allocated in a different code branch from
> > guc_malloc.
> > >
> > > After adding the make_absolute_path() function with psprintf stuff in
> > > path.c file It is giving linking problem in compilation of ecpg. I am
> > not able to find the problem.
> > > So I added another file abspath.c in port which contains these two
> > functions.
> >
> > What errors are you seeing?
>
> If I move the make_absolute_path function from abspath.c to path.c,
> I was getting following linking errors while compiling "ecpg".
>
> ../../../../src/port/libpgport.a(path.o): In function `make_absolute_path':
> /home/hari/postgres/src/port/path.c:795: undefined reference to `psprintf'
> /home/hari/postgres/src/port/path.c:809: undefined reference to `psprintf'
> /home/hari/postgres/src/port/path.c:818: undefined reference to `psprintf'
> /home/hari/postgres/src/port/path.c:830: undefined reference to `psprintf'
> collect2: ld returned 1 exit status
> make[4]: *** [ecpg] Error 1
> make[3]: *** [all-preproc-recurse] Error 2
> make[2]: *** [all-ecpg-recurse] Error 2
> make[1]: *** [all-interfaces-recurse] Error 2
> make: *** [all-src-recurse] Error 2

You didn't show the actual command that is generating the error, but I
assume it is linking ecpg, not creating libecpg. I think the issue is
that path.c is specially handled when it is included in libecpg. Here
is a comment from the libecpg Makefile:

# We use some port modules verbatim, but since we need to
# compile with appropriate options to build a shared lib, we can't
# necessarily use the same object files as the backend uses. Instead,
# symlink the source files in here and build our own object file.

My guess is that libecpg isn't marked as linking to libpgcommon, and
when you called psprintf in path.c, it added a libpgcommon link
requirement.

My guess is that if you compiled common/psprintf.c like port/path.c in
libecpg's Makefile, it would link fine.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

On 20 December 2013 02:02 Bruce Momjian wrote:
> On Thu, Dec 19, 2013 at 05:14:50AM +0000, Haribabu kommi wrote:
> > On 19 December 2013 05:31 Bruce Momjian wrote:
> > > On Wed, Dec 11, 2013 at 10:22:32AM +0000, Haribabu kommi wrote:
> > > > The make_absolute_path() function moving to port is changed in
> > > similar
> > > > way as Bruce Momjian approach. The psprintf is used to store the
> > > error
> > > > string which Occurred in the function. But psprintf is not used
> > > > for storing the absolute path As because it is giving problems in
> > > > freeing
> > > the allocated memory in SelectConfigFiles.
> > > > Because the same memory is allocated in a different code branch
> > > > from
> > > guc_malloc.
> > > >
> > > > After adding the make_absolute_path() function with psprintf
> stuff
> > > > in path.c file It is giving linking problem in compilation of
> > > > ecpg. I am
> > > not able to find the problem.
> > > > So I added another file abspath.c in port which contains these
> two
> > > functions.
> > >
> > > What errors are you seeing?
> >
> > If I move the make_absolute_path function from abspath.c to path.c, I
> > was getting following linking errors while compiling "ecpg".
> >
> > ../../../../src/port/libpgport.a(path.o): In function
> `make_absolute_path':
> > /home/hari/postgres/src/port/path.c:795: undefined reference to
> `psprintf'
> > /home/hari/postgres/src/port/path.c:809: undefined reference to
> `psprintf'
> > /home/hari/postgres/src/port/path.c:818: undefined reference to
> `psprintf'
> > /home/hari/postgres/src/port/path.c:830: undefined reference to
> `psprintf'
> > collect2: ld returned 1 exit status
> > make[4]: *** [ecpg] Error 1
> > make[3]: *** [all-preproc-recurse] Error 2
> > make[2]: *** [all-ecpg-recurse] Error 2
> > make[1]: *** [all-interfaces-recurse] Error 2
> > make: *** [all-src-recurse] Error 2
>
> You didn't show the actual command that is generating the error, but I
> assume it is linking ecpg, not creating libecpg. I think the issue is
> that path.c is specially handled when it is included in libecpg. Here
> is a comment from the libecpg Makefile:
>
> # We use some port modules verbatim, but since we need to
> # compile with appropriate options to build a shared lib, we
> can't
> # necessarily use the same object files as the backend uses.
> Instead,
> # symlink the source files in here and build our own object file.
>
> My guess is that libecpg isn't marked as linking to libpgcommon, and
> when you called psprintf in path.c, it added a libpgcommon link
> requirement.
>
> My guess is that if you compiled common/psprintf.c like port/path.c in
> libecpg's Makefile, it would link fine.

Sorry for not mentioning the command, yes it is giving problem while linking ecpg.

From the compilation I observed as libpgcommon is linking while building ecpg.
I tested the same by using psprintf directly in ecpg code.

I modified the libecpg's Makefile as suggested by you which is attached in the mail,
Still the errors are occurring. Please let me know is there anything missed?

Regards,
Hari babu.

Haribabu kommi escribió:

> From the compilation I observed as libpgcommon is linking while building ecpg.
> I tested the same by using psprintf directly in ecpg code.
>
> I modified the libecpg's Makefile as suggested by you which is attached in the mail,
> Still the errors are occurring. Please let me know is there anything missed?

I don't know what's the cause of the error you're seeing, but IIRC you
can't have a file in src/port depend on src/common functionality (which
psprintf is IIRC). So you need to fix things up so that the psprintf()
doesn't occur in src/port at all. Not sure what's the best way to go
about this; perhaps the whole new function should be in src/common; or
perhaps you need part of it in src/port and the bits with the funny
error reporting in src/common, where psprintf can be called without
issue.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 11/30/13, 6:59 AM, Haribabu kommi wrote:
> To detect provided data and xlog directories are same or not, I reused the
> Existing make_absolute_path() code as follows.

I note that initdb does not detect whether the data and xlog directories
are the same. I think there is no point in addressing this only in
pg_basebackup. If we want to forbid it, it should be done in initdb
foremost.

I'm not sure it's worth the trouble, but if I were to do it, I'd just
stat() the two directories and compare their inodes. That seems much
easier and more robust than comparing path strings.

On Tue, Jan 28, 2014 at 1:17 PM, Peter Eisentraut wrote:

> On 11/30/13, 6:59 AM, Haribabu kommi wrote:
> > To detect provided data and xlog directories are same or not, I reused
> the
> > Existing make_absolute_path() code as follows.
>
> I note that initdb does not detect whether the data and xlog directories
> are the same. I think there is no point in addressing this only in
> pg_basebackup. If we want to forbid it, it should be done in initdb
> foremost.
>

Thanks for pointing it. if the following approach is fine for identifying
the identical directories
then I will do the same for initdb also.

> I'm not sure it's worth the trouble, but if I were to do it, I'd just
> stat() the two directories and compare their inodes. That seems much
> easier and more robust than comparing path strings

stat() is having problems in windows, because of that reason the patch is
written to identify the directories with string comparison.

Regards,
Hari Babu

On Thu, Jan 30, 2014 at 9:37 AM, Haribabu Kommi
<kommi(dot)haribabu(at)gmail(dot)com> wrote:
>
> On Tue, Jan 28, 2014 at 1:17 PM, Peter Eisentraut wrote:
>>
>> On 11/30/13, 6:59 AM, Haribabu kommi wrote:
>> > To detect provided data and xlog directories are same or not, I reused
>> > the
>> > Existing make_absolute_path() code as follows.
>>
>> I note that initdb does not detect whether the data and xlog directories
>> are the same. I think there is no point in addressing this only in
>> pg_basebackup. If we want to forbid it, it should be done in initdb
>> foremost.
>
>
> Thanks for pointing it. if the following approach is fine for identifying
> the identical directories
> then I will do the same for initdb also.

I'm feeling the similar way as Peter. And, ISTM that we need much changes of
source though its benefit is small....

Regards,

--
Fujii Masao

On 1/29/14, 7:37 PM, Haribabu Kommi wrote:
>
> On Tue, Jan 28, 2014 at 1:17 PM, Peter Eisentraut wrote:
>
> On 11/30/13, 6:59 AM, Haribabu kommi wrote:
> > To detect provided data and xlog directories are same or not, I
> reused the
> > Existing make_absolute_path() code as follows.
>
> I note that initdb does not detect whether the data and xlog directories
> are the same. I think there is no point in addressing this only in
> pg_basebackup. If we want to forbid it, it should be done in initdb
> foremost.
>
> Thanks for pointing it. if the following approach is fine for
> identifying the identical directories
> then I will do the same for initdb also.

I wouldn't bother. It's a lot of work for little benefit. Any mistake
a user would make can easily be fixed.

> I'm not sure it's worth the trouble, but if I were to do it, I'd just
> stat() the two directories and compare their inodes. That seems much
> easier and more robust than comparing path strings
>
> stat() is having problems in windows, because of that reason the patch is
> written to identify the directories with string comparison.

If stat() is having problems on Windows, then those problems would need
to be addressed.

I don't think a string comparison is going to be reliable. It can
easily be tricked by using multiple slashes for example, or various
kinds of links or bind mounts. You'd need to put in an awful lot of
work, and it still wouldn't work all the time.

On Sat, Feb 8, 2014 at 12:10 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> On 1/29/14, 7:37 PM, Haribabu Kommi wrote:
> >
> > On Tue, Jan 28, 2014 at 1:17 PM, Peter Eisentraut wrote:
> >
> > On 11/30/13, 6:59 AM, Haribabu kommi wrote:
> > > To detect provided data and xlog directories are same or not, I
> > reused the
> > > Existing make_absolute_path() code as follows.
> >
> > I note that initdb does not detect whether the data and xlog
> directories
> > are the same. I think there is no point in addressing this only in
> > pg_basebackup. If we want to forbid it, it should be done in initdb
> > foremost.
> >
> > Thanks for pointing it. if the following approach is fine for
> > identifying the identical directories
> > then I will do the same for initdb also.
>
> I wouldn't bother. It's a lot of work for little benefit. Any mistake
> a user would make can easily be fixed.

I also felt a lot of work for little benefit but as of now I am not able to
find an easier solution to handle this problem.
can we handle the same later if it really requires?

--
Regards,
Hari Babu
Fujitsu Australia Software Technology

On 2/9/14, 11:16 PM, Haribabu Kommi wrote:
> I also felt a lot of work for little benefit but as of now I am not able
> to find an easier solution to handle this problem.
> can we handle the same later if it really requires?

I think we leave everything as is.