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

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

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

Sounds good! Here are the review comments:

+ 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

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

Sounds good! Here are the review comments:

+ 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.kommi@huawei.com>wrote:

On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

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

Sounds good! Here are the review comments:

+ 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.kommi@huawei.com<mailto:haribabu.kommi@huawei.com>> wrote:
On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com<mailto:haribabu.kommi@huawei.com>> wrote:

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

Sounds good! Here are the review comments:

+ 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.kommi@huawei.com<mailto:haribabu.kommi@huawei.com>> wrote:
On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com<mailto:haribabu.kommi@huawei.com>> wrote:

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

Sounds good! Here are the review comments:

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

I feel no need to prevent, even if user specifies both --pgdata and --xlogdir as same directory
all the transaction log files will be created in the base directory instead of pg_xlog directory.

Given how easy it would be to prevent that, I think we should. It would be an easy misunderstanding to specify that when you actually want it in <wherever>/pg_xlog. Specifying that would be redundant in the first place, but people ca do that, but it
would also be very easy to do it by mistake, and you'd end up with something that's really bad, including a recursive symlink.

Presently with initdb also user can specify both data and xlog directories as same.
To prevent the data directory and xlog directory as same, there is a way in windows (_fullpath api) to get absolute path from a relative path, but I didn't get any such possibilities in linux.
I didn't find any other way to check it, if anyone have any idea regarding this please let me know.

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.kommi@huawei.com> wrote:

on 15 November 2013 17:26 Magnus Hagander wrote:

On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

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

Sounds good! Here are the review comments:

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

I feel no need to prevent, even if user specifies both --pgdata and
--xlogdir as same directory
all the transaction log files will be created in the base directory
instead of pg_xlog directory.

Given how easy it would be to prevent that, I think we should. It would be
an easy misunderstanding to specify that when you actually want it in
<wherever>/pg_xlog. Specifying that would be redundant in the first place,
but people ca do that, but it

would also be very easy to do it by mistake, and you'd end up with
something that's really bad, including a recursive symlink.

Presently with initdb also user can specify both data and xlog directories
as same.

To prevent the data directory and xlog directory as same, there is a way in
windows (_fullpath api) to get absolute path from a relative path, but I
didn’t get any such possibilities in linux.

I didn’t find any other way to check it, if anyone have any idea regarding
this please let me know.

What about make_absolute_path() in miscinit.c?

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 17 November 2013 00:55 Fujii Masao wrote:

On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

on 15 November 2013 17:26 Magnus Hagander wrote:

On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

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

Sounds good! Here are the review comments:

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

I feel no need to prevent, even if user specifies both --pgdata and
--xlogdir as same directory all the transaction log files will be
created in the base directory instead of pg_xlog directory.

Given how easy it would be to prevent that, I think we should. It
would be an easy misunderstanding to specify that when you actually
want it in <wherever>/pg_xlog. Specifying that would be redundant in
the first place, but people ca do that, but it

would also be very easy to do it by mistake, and you'd end up with
something that's really bad, including a recursive symlink.

Presently with initdb also user can specify both data and xlog
directories as same.

To prevent the data directory and xlog directory as same, there is a
way in windows (_fullpath api) to get absolute path from a relative
path, but I didn't get any such possibilities in linux.

I didn't find any other way to check it, if anyone have any idea
regarding this please let me know.

What about make_absolute_path() in miscinit.c?

The make_absoulte_patch() function gets the current working directory and adds
The relative path to CWD, this is not giving proper absolute path.

I have added a new function verify_data_and_xlog_dir_same() which will change the
Current working directory to data directory and gets the CWD and the same way for transaction
log directory. Compare the both data and xlog directories and throw an error. Please check it once.

Is there any other way to identify that both data and xlog directories are pointing to the same
Instead of comparing both absolute paths?

Updated patch attached in the mail.

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.kommi@huawei.com> wrote:

on 15 November 2013 17:26 Magnus Hagander wrote:

On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

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

Sounds good! Here are the review comments:

Don't we need to prevent users from specifying the same

directory

in both --pgdata and --xlogdir?

I feel no need to prevent, even if user specifies both --pgdata

and

--xlogdir as same directory all the transaction log files will be
created in the base directory instead of pg_xlog directory.

Given how easy it would be to prevent that, I think we should. It
would be an easy misunderstanding to specify that when you

actually

want it in <wherever>/pg_xlog. Specifying that would be redundant
in the first place, but people ca do that, but it

would also be very easy to do it by mistake, and you'd end up with
something that's really bad, including a recursive symlink.

Presently with initdb also user can specify both data and xlog
directories as same.

To prevent the data directory and xlog directory as same, there is

a

way in windows (_fullpath api) to get absolute path from a relative
path, but I didn't get any such possibilities in linux.

I didn't find any other way to check it, if anyone have any idea
regarding this please let me know.

What about make_absolute_path() in miscinit.c?

The make_absoulte_patch() function gets the current working directory
and adds The relative path to CWD, this is not giving proper absolute
path.

I have added a new function verify_data_and_xlog_dir_same() which will
change the Current working directory to data directory and gets the CWD
and the same way for transaction log directory. Compare the both data
and xlog directories and throw an error. Please check it once.

Is there any other way to identify that both data and xlog directories
are pointing to the same Instead of comparing both absolute paths?

Updated patch attached in the mail.

Failure when the xlogdir doesn't exist is fixed in the updated patch attached in the mail.

Regards,
Hari babu.

On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 18 November 2013 11:19 Haribabu kommi wrote:

On 17 November 2013 00:55 Fujii Masao wrote:

On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

on 15 November 2013 17:26 Magnus Hagander wrote:

On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

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

Sounds good! Here are the review comments:

Don't we need to prevent users from specifying the same

directory

in both --pgdata and --xlogdir?

I feel no need to prevent, even if user specifies both --pgdata

and

--xlogdir as same directory all the transaction log files will be
created in the base directory instead of pg_xlog directory.

Given how easy it would be to prevent that, I think we should. It
would be an easy misunderstanding to specify that when you

actually

want it in <wherever>/pg_xlog. Specifying that would be redundant
in the first place, but people ca do that, but it

would also be very easy to do it by mistake, and you'd end up with
something that's really bad, including a recursive symlink.

Presently with initdb also user can specify both data and xlog
directories as same.

To prevent the data directory and xlog directory as same, there is

a

way in windows (_fullpath api) to get absolute path from a relative
path, but I didn't get any such possibilities in linux.

I didn't find any other way to check it, if anyone have any idea
regarding this please let me know.

What about make_absolute_path() in miscinit.c?

The make_absoulte_patch() function gets the current working directory
and adds The relative path to CWD, this is not giving proper absolute
path.

I have added a new function verify_data_and_xlog_dir_same() which will
change the Current working directory to data directory and gets the CWD
and the same way for transaction log directory. Compare the both data
and xlog directories and throw an error. Please check it once.

Is there any other way to identify that both data and xlog directories
are pointing to the same Instead of comparing both absolute paths?

Updated patch attached in the mail.

Failure when the xlogdir doesn't exist is fixed in the updated patch attached in the mail.

Thanks for updating the patch!

With the patch, when I specify the same directory in both -D and --xlogdir,
I got the following error.

$ cd /tmp
$ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
pg_basebackup: could not change directory to "hoge": No such file or directory

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 18 November 2013 18:45 Fujii Masao wrote:

On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 18 November 2013 11:19 Haribabu kommi wrote:

On 17 November 2013 00:55 Fujii Masao wrote:

On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

on 15 November 2013 17:26 Magnus Hagander wrote:

On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

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

Sounds good! Here are the review comments:

Don't we need to prevent users from specifying the same

directory

in both --pgdata and --xlogdir?

I feel no need to prevent, even if user specifies both --pgdata

and

--xlogdir as same directory all the transaction log files will
be created in the base directory instead of pg_xlog directory.

Given how easy it would be to prevent that, I think we should.

It

would be an easy misunderstanding to specify that when you

actually

want it in <wherever>/pg_xlog. Specifying that would be
redundant in the first place, but people ca do that, but it

would also be very easy to do it by mistake, and you'd end up
with something that's really bad, including a recursive symlink.

Presently with initdb also user can specify both data and xlog
directories as same.

To prevent the data directory and xlog directory as same, there
is

a

way in windows (_fullpath api) to get absolute path from a
relative path, but I didn't get any such possibilities in linux.

I didn't find any other way to check it, if anyone have any idea
regarding this please let me know.

What about make_absolute_path() in miscinit.c?

The make_absoulte_patch() function gets the current working

directory

and adds The relative path to CWD, this is not giving proper

absolute

path.

I have added a new function verify_data_and_xlog_dir_same() which
will change the Current working directory to data directory and gets
the CWD and the same way for transaction log directory. Compare the
both data and xlog directories and throw an error. Please check it

once.

Is there any other way to identify that both data and xlog
directories are pointing to the same Instead of comparing both

absolute paths?

Updated patch attached in the mail.

Failure when the xlogdir doesn't exist is fixed in the updated patch

attached in the mail.

Thanks for updating the patch!

With the patch, when I specify the same directory in both -D and --
xlogdir, I got the following error.

$ cd /tmp
$ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
pg_basebackup: could not change directory to "hoge": No such file or
directory

Thanks. After finding the xlog directory absolute path returning back to current working
Directory is missed, because of this reason it is failing while finding the absolute
Path for data directory. Apart from this change the code is reorganized a bit.

Updated patch is attached in the mail. Please review it once.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 18 November 2013 18:45 Fujii Masao wrote:

On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 18 November 2013 11:19 Haribabu kommi wrote:

On 17 November 2013 00:55 Fujii Masao wrote:

On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

on 15 November 2013 17:26 Magnus Hagander wrote:

On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

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

Sounds good! Here are the review comments:

Don't we need to prevent users from specifying the same

directory

in both --pgdata and --xlogdir?

I feel no need to prevent, even if user specifies both --pgdata

and

--xlogdir as same directory all the transaction log files will
be created in the base directory instead of pg_xlog directory.

Given how easy it would be to prevent that, I think we should.

It

would be an easy misunderstanding to specify that when you

actually

want it in <wherever>/pg_xlog. Specifying that would be
redundant in the first place, but people ca do that, but it

would also be very easy to do it by mistake, and you'd end up
with something that's really bad, including a recursive symlink.

Presently with initdb also user can specify both data and xlog
directories as same.

To prevent the data directory and xlog directory as same, there
is

a

way in windows (_fullpath api) to get absolute path from a
relative path, but I didn't get any such possibilities in linux.

I didn't find any other way to check it, if anyone have any idea
regarding this please let me know.

What about make_absolute_path() in miscinit.c?

The make_absoulte_patch() function gets the current working

directory

and adds The relative path to CWD, this is not giving proper

absolute

path.

I have added a new function verify_data_and_xlog_dir_same() which
will change the Current working directory to data directory and gets
the CWD and the same way for transaction log directory. Compare the
both data and xlog directories and throw an error. Please check it

once.

Is there any other way to identify that both data and xlog
directories are pointing to the same Instead of comparing both

absolute paths?

Updated patch attached in the mail.

Failure when the xlogdir doesn't exist is fixed in the updated patch

attached in the mail.

Thanks for updating the patch!

With the patch, when I specify the same directory in both -D and --
xlogdir, I got the following error.

$ cd /tmp
$ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
pg_basebackup: could not change directory to "hoge": No such file or
directory

Thanks. After finding the xlog directory absolute path returning back to current working
Directory is missed, because of this reason it is failing while finding the absolute
Path for data directory. Apart from this change the code is reorganized a bit.

Updated patch is attached in the mail. Please review it once.

Thanks for newer version of the patch!

I found that the empty base directory is created and remains even when the same
directory is specified in both -D and --xlogdir and the error occurs.
I think that it's
better to throw an error in that case before creating any new
directory. Thought?

+ xlogdir = get_absolute_path(xlog_dir);

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

Regards,

--
Fujii Masao

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 18 November 2013 23:30 Fujii Masao wrote:

On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 18 November 2013 18:45 Fujii Masao wrote:

On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 18 November 2013 11:19 Haribabu kommi wrote:

On 17 November 2013 00:55 Fujii Masao wrote:

On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

on 15 November 2013 17:26 Magnus Hagander wrote:

On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

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

pg_xlog.

Sounds good! Here are the review comments:

Don't we need to prevent users from specifying the same

directory

in both --pgdata and --xlogdir?

I feel no need to prevent, even if user specifies both
--pgdata

and

--xlogdir as same directory all the transaction log files
will be created in the base directory instead of pg_xlog

directory.

Given how easy it would be to prevent that, I think we should.

It

would be an easy misunderstanding to specify that when you

actually

want it in <wherever>/pg_xlog. Specifying that would be
redundant in the first place, but people ca do that, but it

would also be very easy to do it by mistake, and you'd end up
with something that's really bad, including a recursive

symlink.

Presently with initdb also user can specify both data and

xlog

directories as same.

To prevent the data directory and xlog directory as same,
there is

a

way in windows (_fullpath api) to get absolute path from a
relative path, but I didn't get any such possibilities in

linux.

I didn't find any other way to check it, if anyone have any
idea regarding this please let me know.

What about make_absolute_path() in miscinit.c?

The make_absoulte_patch() function gets the current working

directory

and adds The relative path to CWD, this is not giving proper

absolute

path.

I have added a new function verify_data_and_xlog_dir_same() which
will change the Current working directory to data directory and
gets the CWD and the same way for transaction log directory.
Compare the both data and xlog directories and throw an error.
Please check it

once.

Is there any other way to identify that both data and xlog
directories are pointing to the same Instead of comparing both

absolute paths?

Updated patch attached in the mail.

Failure when the xlogdir doesn't exist is fixed in the updated
patch

attached in the mail.

Thanks for updating the patch!

With the patch, when I specify the same directory in both -D and --
xlogdir, I got the following error.

$ cd /tmp
$ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
pg_basebackup: could not change directory to "hoge": No such file or
directory

Thanks. After finding the xlog directory absolute path returning back
to current working Directory is missed, because of this reason it is
failing while finding the absolute Path for data directory. Apart

from this change the code is reorganized a bit.

Updated patch is attached in the mail. Please review it once.

Thanks for newer version of the patch!

I found that the empty base directory is created and remains even when
the same directory is specified in both -D and --xlogdir and the error
occurs.
I think that it's
better to throw an error in that case before creating any new directory.
Thought?

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Nov 19, 2013 at 9:14 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 18 November 2013 23:30 Fujii Masao wrote:

On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 18 November 2013 18:45 Fujii Masao wrote:

On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 18 November 2013 11:19 Haribabu kommi wrote:

On 17 November 2013 00:55 Fujii Masao wrote:

On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

on 15 November 2013 17:26 Magnus Hagander wrote:

On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

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

pg_xlog.

Sounds good! Here are the review comments:

Don't we need to prevent users from specifying the same

directory

in both --pgdata and --xlogdir?

I feel no need to prevent, even if user specifies both
--pgdata

and

--xlogdir as same directory all the transaction log files
will be created in the base directory instead of pg_xlog

directory.

Given how easy it would be to prevent that, I think we should.

It

would be an easy misunderstanding to specify that when you

actually

want it in <wherever>/pg_xlog. Specifying that would be
redundant in the first place, but people ca do that, but it

would also be very easy to do it by mistake, and you'd end up
with something that's really bad, including a recursive

symlink.

Presently with initdb also user can specify both data and

xlog

directories as same.

To prevent the data directory and xlog directory as same,
there is

a

way in windows (_fullpath api) to get absolute path from a
relative path, but I didn't get any such possibilities in

linux.

I didn't find any other way to check it, if anyone have any
idea regarding this please let me know.

What about make_absolute_path() in miscinit.c?

The make_absoulte_patch() function gets the current working

directory

and adds The relative path to CWD, this is not giving proper

absolute

path.

I have added a new function verify_data_and_xlog_dir_same() which
will change the Current working directory to data directory and
gets the CWD and the same way for transaction log directory.
Compare the both data and xlog directories and throw an error.
Please check it

once.

Is there any other way to identify that both data and xlog
directories are pointing to the same Instead of comparing both

absolute paths?

Updated patch attached in the mail.

Failure when the xlogdir doesn't exist is fixed in the updated
patch

attached in the mail.

Thanks for updating the patch!

With the patch, when I specify the same directory in both -D and --
xlogdir, I got the following error.

$ cd /tmp
$ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
pg_basebackup: could not change directory to "hoge": No such file or
directory

Thanks. After finding the xlog directory absolute path returning back
to current working Directory is missed, because of this reason it is
failing while finding the absolute Path for data directory. Apart

from this change the code is reorganized a bit.

Updated patch is attached in the mail. Please review it once.

Thanks for newer version of the patch!

I found that the empty base directory is created and remains even when
the same directory is specified in both -D and --xlogdir and the error
occurs.
I think that it's
better to throw an error in that case before creating any new directory.
Thought?

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Fujii Masao <masao.fujii@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

On Tue, Nov 19, 2013 at 9:14 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 18 November 2013 23:30 Fujii Masao wrote:

On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 18 November 2013 18:45 Fujii Masao wrote:

On Mon, Nov 18, 2013 at 6:31 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 18 November 2013 11:19 Haribabu kommi wrote:

On 17 November 2013 00:55 Fujii Masao wrote:

On Sat, Nov 16, 2013 at 2:27 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

on 15 November 2013 17:26 Magnus Hagander wrote:

On Fri, Nov 15, 2013 at 12:10 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

On 14 November 2013 23:59 Fujii Masao wrote:

On Thu, Nov 14, 2013 at 9:08 PM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

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

pg_xlog.

Sounds good! Here are the review comments:

Don't we need to prevent users from specifying the same

directory

in both --pgdata and --xlogdir?

I feel no need to prevent, even if user specifies both
--pgdata

and

--xlogdir as same directory all the transaction log files
will be created in the base directory instead of pg_xlog

directory.

Given how easy it would be to prevent that, I think we should.

It

would be an easy misunderstanding to specify that when you

actually

want it in <wherever>/pg_xlog. Specifying that would be
redundant in the first place, but people ca do that, but it

would also be very easy to do it by mistake, and you'd end up
with something that's really bad, including a recursive

symlink.

Presently with initdb also user can specify both data and

xlog

directories as same.

To prevent the data directory and xlog directory as same,
there is

a

way in windows (_fullpath api) to get absolute path from a
relative path, but I didn't get any such possibilities in

linux.

I didn't find any other way to check it, if anyone have any
idea regarding this please let me know.

What about make_absolute_path() in miscinit.c?

The make_absoulte_patch() function gets the current working

directory

and adds The relative path to CWD, this is not giving proper

absolute

path.

I have added a new function verify_data_and_xlog_dir_same() which
will change the Current working directory to data directory and
gets the CWD and the same way for transaction log directory.
Compare the both data and xlog directories and throw an error.
Please check it

once.

Is there any other way to identify that both data and xlog
directories are pointing to the same Instead of comparing both

absolute paths?

Updated patch attached in the mail.

Failure when the xlogdir doesn't exist is fixed in the updated
patch

attached in the mail.

Thanks for updating the patch!

With the patch, when I specify the same directory in both -D and --
xlogdir, I got the following error.

$ cd /tmp
$ pg_basebackup -D hoge --xlogdir=/tmp/hoge -X fetch
pg_basebackup: could not change directory to "hoge": No such file or
directory

Thanks. After finding the xlog directory absolute path returning back
to current working Directory is missed, because of this reason it is
failing while finding the absolute Path for data directory. Apart

from this change the code is reorganized a bit.

Updated patch is attached in the mail. Please review it once.

Thanks for newer version of the patch!

I found that the empty base directory is created and remains even when
the same directory is specified in both -D and --xlogdir and the error
occurs.
I think that it's
better to throw an error in that case before creating any new directory.
Thought?

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.kommi@huawei.com> wrote:

On 18 November 2013 23:30 Fujii Masao wrote:

On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

Thanks for newer version of the patch!

I found that the empty base directory is created and remains even
when the same directory is specified in both -D and --xlogdir and

the

error occurs.
I think that it's
better to throw an error in that case before creating any new

directory.

Thought?

By creating the base directory only the patch finds whether provided
base and Xlog directories are same or not? To solve this problem
following options are possible

1. No problem as it is just an empty base directory, so it can be

reused in the next time

Leave it as it is.
2. Once the error is identified, the base directory can be deleted.
3. write a new function to remove the parent references from the

provided path to identify

the absolute path used for detecting base and Xlog directories are

same or not?

Please provide your suggestions.

+ xlogdir = get_absolute_path(xlog_dir);

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

It is required. As user can provide the path as

/home/installation/bin/../bin/data.

The provided path is considered as absolute path only but while
comparing the same With data directory path it will not match.

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

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

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

I tried using of stat'ing in two directories, which is having a problem in windows.
So modified old approach to detect limited errors. Updated patch attached.
This will detect and throw an error in the following scenarios.
1. pg_basebackup -D /home/data --xlogdir=/home/data
2. pg_basebackup -D data --xlogdir=/home/data -- home is the CWD
3. pg_basebackup -D ../data --xlogdir=/data -- home is the CWD

Please let me know your suggestions.

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.kommi@huawei.com> wrote:

On 18 November 2013 23:30 Fujii Masao wrote:

On Tue, Nov 19, 2013 at 12:01 AM, Haribabu kommi
<haribabu.kommi@huawei.com> wrote:

Thanks for newer version of the patch!

I found that the empty base directory is created and remains even
when the same directory is specified in both -D and --xlogdir and

the

error occurs.
I think that it's
better to throw an error in that case before creating any new

directory.

Thought?

By creating the base directory only the patch finds whether provided
base and Xlog directories are same or not? To solve this problem
following options are possible

1. No problem as it is just an empty base directory, so it can be

reused in the next time

Leave it as it is.
2. Once the error is identified, the base directory can be deleted.
3. write a new function to remove the parent references from the

provided path to identify

the absolute path used for detecting base and Xlog directories are

same or not?

Please provide your suggestions.

+ xlogdir = get_absolute_path(xlog_dir);

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

It is required. As user can provide the path as

/home/installation/bin/../bin/data.

The provided path is considered as absolute path only but while
comparing the same With data directory path it will not match.

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

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

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

I tried using of stat'ing in two directories, which is having a problem in windows.
So modified old approach to detect limited errors. Updated patch attached.
This will detect and throw an error in the following scenarios.
1. pg_basebackup -D /home/data --xlogdir=/home/data
2. pg_basebackup -D data --xlogdir=/home/data -- home is the CWD
3. pg_basebackup -D ../data --xlogdir=/data -- home is the CWD

Please let me know your suggestions.

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