Re: [HACKERS] Streaming basebackups vs pg_stat_tmp

[Magnus Hagander]

On Tue, Nov 8, 2016 at 1:28 PM, David Steele  wrote:

> Hi Magnus,
>
> On 11/7/16 2:07 PM, Magnus Hagander wrote:
>
>> On Sat, Oct 29, 2016 at 4:12 PM, Michael Paquier
>> Indeed, giving the attached for REL9_6_STABLE. You could as well have
>> a test for pg_stat_tmp but honestly that's not worth it. One thing I
>> have noticed is that the patch does not use _tarWriteDir() for
>> pg_xlog. I think it should even if that's not addressing directly a
>> bug...
>>
>> Applied and backported, thanks. Backported to 9.4, as this is where that
>> exclusion code appeared.
>>
>
> I reviewed the three back-patches and they look sensible to me.


Thanks!



> I did not backport the tests, as we don't have the $node stuff available
>> in 9.5 and earlier.
>>
>
> That's unfortunate but the changes are pretty straightforward and the
> testing is as good as it was before...


Yeah, that's my thinking as well. It would be nice, but not worth the
effort.

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

Re: [HACKERS] Streaming basebackups vs pg_stat_tmp

[David Steele]

Hi Magnus,

On 11/7/16 2:07 PM, Magnus Hagander wrote:

On Sat, Oct 29, 2016 at 4:12 PM, Michael Paquier
Indeed, giving the attached for REL9_6_STABLE. You could as well have
a test for pg_stat_tmp but honestly that's not worth it. One thing I
have noticed is that the patch does not use _tarWriteDir() for
pg_xlog. I think it should even if that's not addressing directly a
bug...

Applied and backported, thanks. Backported to 9.4, as this is where that
exclusion code appeared.


I reviewed the three back-patches and they look sensible to me.


I did not backport the tests, as we don't have the $node stuff available
in 9.5 and earlier.


That's unfortunate but the changes are pretty straightforward and the 
testing is as good as it was before...


--
-David
da...@pgmasters.net


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

Re: [HACKERS] Streaming basebackups vs pg_stat_tmp

[Magnus Hagander]

On Sat, Oct 29, 2016 at 4:12 PM, Michael Paquier 
wrote:

> On Fri, Oct 28, 2016 at 9:57 PM, David Steele  wrote:
> > On 10/28/16 3:49 PM, Magnus Hagander wrote:
> > The change from 10 to 11 increases the tests that are skipped on Windows,
> > which is necessary because one extra symlink test is added.
> >
> > I think you need:
> >
> > [...]
> >
> > The rest of the tests are for exclusions.
>
> Indeed, giving the attached for REL9_6_STABLE. You could as well have
> a test for pg_stat_tmp but honestly that's not worth it. One thing I
> have noticed is that the patch does not use _tarWriteDir() for
> pg_xlog. I think it should even if that's not addressing directly a
> bug...
>


Applied and backported, thanks. Backported to 9.4, as this is where that
exclusion code appeared.

I did not backport the tests, as we don't have the $node stuff available in
9.5 and earlier.

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

Re: [HACKERS] Streaming basebackups vs pg_stat_tmp

[Michael Paquier]

On Fri, Oct 28, 2016 at 9:57 PM, David Steele  wrote:
> On 10/28/16 3:49 PM, Magnus Hagander wrote:
> The change from 10 to 11 increases the tests that are skipped on Windows,
> which is necessary because one extra symlink test is added.
>
> I think you need:
>
> [...]
>
> The rest of the tests are for exclusions.

Indeed, giving the attached for REL9_6_STABLE. You could as well have
a test for pg_stat_tmp but honestly that's not worth it. One thing I
have noticed is that the patch does not use _tarWriteDir() for
pg_xlog. I think it should even if that's not addressing directly a
bug...
-- 
Michael
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index da9b7a6..426f6c8 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -57,6 +57,8 @@ static bool sendFile(char *readfilename, char *tarfilename,
 static void sendFileWithContent(const char *filename, const char *content);
 static void _tarWriteHeader(const char *filename, const char *linktarget,
 struct stat * statbuf);
+static int64 _tarWriteDir(const char *pathbuf, int basepathlen, struct stat * statbuf,
+			 bool sizeonly);
 static void send_int8_string(StringInfoData *buf, int64 intval);
 static void SendBackupHeader(List *tablespaces);
 static void base_backup_cleanup(int code, Datum arg);
@@ -969,9 +971,7 @@ sendDir(char *path, int basepathlen, bool sizeonly, List *tablespaces,
 		if ((statrelpath != NULL && strcmp(pathbuf, statrelpath) == 0) ||
 		  strncmp(de->d_name, PG_STAT_TMP_DIR, strlen(PG_STAT_TMP_DIR)) == 0)
 		{
-			if (!sizeonly)
-_tarWriteHeader(pathbuf + basepathlen + 1, NULL, );
-			size += 512;
+			size += _tarWriteDir(pathbuf, basepathlen, , sizeonly);
 			continue;
 		}
 
@@ -981,9 +981,7 @@ sendDir(char *path, int basepathlen, bool sizeonly, List *tablespaces,
 		 */
 		if (strcmp(de->d_name, "pg_replslot") == 0)
 		{
-			if (!sizeonly)
-_tarWriteHeader(pathbuf + basepathlen + 1, NULL, );
-			size += 512;		/* Size of the header just added */
+			size += _tarWriteDir(pathbuf, basepathlen, , sizeonly);
 			continue;
 		}
 
@@ -994,18 +992,8 @@ sendDir(char *path, int basepathlen, bool sizeonly, List *tablespaces,
 		 */
 		if (strcmp(pathbuf, "./pg_xlog") == 0)
 		{
-			if (!sizeonly)
-			{
-/* If pg_xlog is a symlink, write it as a directory anyway */
-#ifndef WIN32
-if (S_ISLNK(statbuf.st_mode))
-#else
-if (pgwin32_is_junction(pathbuf))
-#endif
-	statbuf.st_mode = S_IFDIR | S_IRWXU;
-_tarWriteHeader(pathbuf + basepathlen + 1, NULL, );
-			}
-			size += 512;		/* Size of the header just added */
+			/* If pg_xlog is a symlink, write it as a directory anyway */
+			size += _tarWriteDir(pathbuf, basepathlen, , sizeonly);
 
 			/*
 			 * Also send archive_status directory (by hackishly reusing
@@ -1248,6 +1236,30 @@ _tarWriteHeader(const char *filename, const char *linktarget,
 }
 
 /*
+ * Write tar header for a directory.  If the entry in statbuf is a link then
+ * write it as a directory anyway.
+ */
+static int64
+_tarWriteDir(const char *pathbuf, int basepathlen, struct stat * statbuf,
+			 bool sizeonly)
+{
+	if (sizeonly)
+		/* Directory headers are always 512 bytes */
+		return 512;
+
+	/* If symlink, write it as a directory anyway */
+#ifndef WIN32
+	if (S_ISLNK(statbuf->st_mode))
+#else
+	if (pgwin32_is_junction(pathbuf))
+#endif
+		statbuf->st_mode = S_IFDIR | S_IRWXU;
+
+	_tarWriteHeader(pathbuf + basepathlen + 1, NULL, statbuf);
+	return 512;
+}
+
+/*
  * Increment the network transfer counter by the given number of bytes,
  * and sleep if necessary to comply with the requested network transfer
  * rate.
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 6c33936..a83f3af 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -4,7 +4,7 @@ use Cwd;
 use Config;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 51;
+use Test::More tests => 52;
 
 program_help_ok('pg_basebackup');
 program_version_ok('pg_basebackup');
@@ -102,7 +102,17 @@ unlink "$pgdata/$superlongname";
 # skip on Windows.
 SKIP:
 {
-	skip "symlinks not supported on Windows", 10 if ($windows_os);
+	skip "symlinks not supported on Windows", 11 if ($windows_os);
+
+	# Move pg_replslot out of $pgdata and create a symlink to it.
+	$node->stop;
+
+	rename("$pgdata/pg_replslot", "$tempdir/pg_replslot")
+		   or BAIL_OUT "could not move $pgdata/pg_replslot";
+	symlink("$tempdir/pg_replslot", "$pgdata/pg_replslot")
+		   or BAIL_OUT "could not symlink to $pgdata/pg_replslot";
+
+	$node->start;
 
 	# Create a temporary directory in the system location and symlink it
 	# to our physical temp location.  That way we can use shorter names
@@ -140,6 +150,8 @@ SKIP:
 		"tablespace symlink was updated");
 	closedir $dh;
 
+	ok(-d "$tempdir/backup1/pg_replslot", 'pg_replslot symlink copied as directory');
+
 	mkdir 

Re: [HACKERS] Streaming basebackups vs pg_stat_tmp

[David Steele]

On 10/28/16 3:49 PM, Magnus Hagander wrote:

On Fri, Oct 28, 2016 at 2:44 PM, David Steele > wrote:
The patch looks sane to me, but I think it would be good to
backpatch the TAP test from the exclusion patch that tests
pg_replslot as a symlink.

So that's the test that's in that same patch, 6ad8ac60, right? How much
of the code for that is actually needed? (like the row which changes a
10 to a 11? which probably means something, but is it relevant here?) Or
all of it?


The change from 10 to 11 increases the tests that are skipped on 
Windows, which is necessary because one extra symlink test is added.


I think you need:

-use Test::More tests => 54;
+use Test::More tests => 55;

and:

 SKIP:
 {
-   skip "symlinks not supported on Windows", 10 if ($windows_os);
+   skip "symlinks not supported on Windows", 11 if ($windows_os);
+
+   # Move pg_replslot out of $pgdata and create a symlink to it.
+   $node->stop;
+
+   rename("$pgdata/pg_replslot", "$tempdir/pg_replslot")
+   or BAIL_OUT "could not move $pgdata/pg_replslot";
+   symlink("$tempdir/pg_replslot", "$pgdata/pg_replslot")
+   or BAIL_OUT "could not symlink to $pgdata/pg_replslot";
+
+   $node->start;

# Create a temporary directory in the system location and symlink it
# to our physical temp location.  That way we can use shorter names
@@ -148,6 +186,8 @@ SKIP:
"tablespace symlink was updated");
closedir $dh;

+   ok(-d "$tempdir/backup1/pg_replslot", 'pg_replslot symlink copied as 
directory');

+
mkdir "$tempdir/tbl=spc2";

The rest of the tests are for exclusions.

--
-David
da...@pgmasters.net


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

Re: [HACKERS] Streaming basebackups vs pg_stat_tmp

[Magnus Hagander]

On Fri, Oct 28, 2016 at 2:44 PM, David Steele  wrote:

> On 10/28/16 11:53 AM, Magnus Hagander wrote:
>
>> In 9.6 and earlier, if you change pg_stat_tmp to be a symlink,
>> basebackups no longer work. That's because we create symlink entry in
>> the tarfile for it instead of an empty directory, but with no data,
>> which Breaks Everything (TM).
>>
>> This was fixed in head in 6ad8ac60, which introduced "more excludes",
>> due to the refactoring. That commit message refers to it also fixing
>> this bug, but it seems the bugfix was never backpatched.
>>
>> Or did I miss something?
>>
>
> I don't think so.  I guess it got lost in the CF rush and also slipped my
> mind when I reviewed the final commit.
>
> Attached patch fixds this (based on 9.5 which is where I ran into it,
>> but it needs to go in other back branches as well) by bringing back a
>> (modified) version of the functoin _tarWriteDir() to the back branches.
>>
>> I'd appreciate a look-over before committing, but it works fine in my
>> tests.
>>
>
> The patch looks sane to me, but I think it would be good to backpatch the
> TAP test from the exclusion patch that tests pg_replslot as a symlink.


So that's the test that's in that same patch, 6ad8ac60, right? How much of
the code for that is actually needed? (like the row which changes a 10 to a
11? which probably means something, but is it relevant here?) Or all of it?

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

Re: [HACKERS] Streaming basebackups vs pg_stat_tmp

[David Steele]

On 10/28/16 11:53 AM, Magnus Hagander wrote:

In 9.6 and earlier, if you change pg_stat_tmp to be a symlink,
basebackups no longer work. That's because we create symlink entry in
the tarfile for it instead of an empty directory, but with no data,
which Breaks Everything (TM).

This was fixed in head in 6ad8ac60, which introduced "more excludes",
due to the refactoring. That commit message refers to it also fixing
this bug, but it seems the bugfix was never backpatched.

Or did I miss something?


I don't think so.  I guess it got lost in the CF rush and also slipped 
my mind when I reviewed the final commit.



Attached patch fixds this (based on 9.5 which is where I ran into it,
but it needs to go in other back branches as well) by bringing back a
(modified) version of the functoin _tarWriteDir() to the back branches.

I'd appreciate a look-over before committing, but it works fine in my tests.


The patch looks sane to me, but I think it would be good to backpatch 
the TAP test from the exclusion patch that tests pg_replslot as a symlink.


--
-David
da...@pgmasters.net


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

[HACKERS] Streaming basebackups vs pg_stat_tmp

[Magnus Hagander]

In 9.6 and earlier, if you change pg_stat_tmp to be a symlink, basebackups
no longer work. That's because we create symlink entry in the tarfile for
it instead of an empty directory, but with no data, which Breaks Everything
(TM).

This was fixed in head in 6ad8ac60, which introduced "more excludes", due
to the refactoring. That commit message refers to it also fixing this bug,
but it seems the bugfix was never backpatched.

Or did I miss something?

Attached patch fixds this (based on 9.5 which is where I ran into it, but
it needs to go in other back branches as well) by bringing back a
(modified) version of the functoin _tarWriteDir() to the back branches.

I'd appreciate a look-over before committing, but it works fine in my tests.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 6120c8f..c4e3a08 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -57,6 +57,8 @@ static bool sendFile(char *readfilename, char *tarfilename,
 static void sendFileWithContent(const char *filename, const char *content);
 static void _tarWriteHeader(const char *filename, const char *linktarget,
 struct stat * statbuf);
+static int64 _tarWriteDir(const char *pathbuf, int basepathlen, struct stat * statbuf,
+			 bool sizeonly);
 static void send_int8_string(StringInfoData *buf, int64 intval);
 static void SendBackupHeader(List *tablespaces);
 static void base_backup_cleanup(int code, Datum arg);
@@ -966,9 +968,7 @@ sendDir(char *path, int basepathlen, bool sizeonly, List *tablespaces,
 		if ((statrelpath != NULL && strcmp(pathbuf, statrelpath) == 0) ||
 		  strncmp(de->d_name, PG_STAT_TMP_DIR, strlen(PG_STAT_TMP_DIR)) == 0)
 		{
-			if (!sizeonly)
-_tarWriteHeader(pathbuf + basepathlen + 1, NULL, );
-			size += 512;
+			size += _tarWriteDir(pathbuf, basepathlen, , sizeonly);
 			continue;
 		}
 
@@ -978,9 +978,7 @@ sendDir(char *path, int basepathlen, bool sizeonly, List *tablespaces,
 		 */
 		if (strcmp(de->d_name, "pg_replslot") == 0)
 		{
-			if (!sizeonly)
-_tarWriteHeader(pathbuf + basepathlen + 1, NULL, );
-			size += 512;		/* Size of the header just added */
+			size += _tarWriteDir(pathbuf, basepathlen, , sizeonly);
 			continue;
 		}
 
@@ -1245,6 +1243,30 @@ _tarWriteHeader(const char *filename, const char *linktarget,
 }
 
 /*
+ * Write tar header for a directory.  If the entry in statbuf is a link then
+ * write it as a directory anyway.
+ */
+static int64
+_tarWriteDir(const char *pathbuf, int basepathlen, struct stat * statbuf,
+			 bool sizeonly)
+{
+	if (sizeonly)
+		/* Directory headers are always 512 bytes */
+		return 512;
+
+	/* If symlink, write it as a directory anyway */
+#ifndef WIN32
+	if (S_ISLNK(statbuf->st_mode))
+#else
+	if (pgwin32_is_junction(pathbuf))
+#endif
+		statbuf->st_mode = S_IFDIR | S_IRWXU;
+
+	_tarWriteHeader(pathbuf + basepathlen + 1, NULL, statbuf);
+	return 512;
+}
+
+/*
  * Increment the network transfer counter by the given number of bytes,
  * and sleep if necessary to comply with the requested network transfer
  * rate.

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