Subject: Re: pg_basebackup check vs Windows file path limits

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 4 Jul 2023 22:54:54 +0200

> On 4 Jul 2023, at 20:19, Andrew Dunstan <[email-masked]> wrote:

> But sadly we're kinda back where we started. fairywren is failing on REL_16_STABLE. Before the changes the failure occurred because the test script was unable to create the file with a path > 255. Now that we have a way to create the file the test for pg_basebackup to reject files with names > 100 fails, I presume because the server can't actually see the file. At this stage I'm thinking the best thing would be to skip the test altogether on windows if the path is longer than 255.

That does sound like a fairly large hammer for a nail small enough that we
should be able to fix it, but I don't have any other good ideas off the cuff.

--
Daniel Gustafsson

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 5 Jul 2023 08:49:39 -0400

On 2023-07-04 Tu 16:54, Daniel Gustafsson wrote:
>> On 4 Jul 2023, at 20:19, Andrew Dunstan<[email-masked]>  wrote:

Not sure it's such a big hammer. Here's a patch.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index e0009c8531..ee0d03512c 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -311,20 +311,22 @@ $node->command_fails(
 	'-T with invalid format fails');
 
 # Tar format doesn't support filenames longer than 100 bytes.
-# Create the test file via a short name directory so it doesn't blow the
-# Windows path limit.
-my $lftmp = PostgreSQL::Test::Utils::tempdir_short;
-dir_symlink "$pgdata", "$lftmp/pgdata";
-my $superlongname = "superlongname_" . ("x" x 100);
-my $superlongpath = "$lftmp/pgdata/$superlongname";
+SKIP:
+{
+	my $superlongname = "superlongname_" . ("x" x 100);
+	my $superlongpath = "$pgdata/$superlongname";
 
-open my $file, '>', "$superlongpath"
-  or die "unable to create file $superlongpath";
-close $file;
-$node->command_fails(
-	[ @pg_basebackup_defs, '-D', "$tempdir/tarbackup_l1", '-Ft' ],
-	'pg_basebackup tar with long name fails');
-unlink "$superlongpath";
+	skip "File path too long", 1
+	  if $windows_os && length($superlongpath) > 255;
+
+	open my $file, '>', "$superlongpath"
+	  or die "unable to create file $superlongpath";
+	close $file;
+	$node->command_fails(
+		[ @pg_basebackup_defs, '-D', "$tempdir/tarbackup_l1", '-Ft' ],
+		'pg_basebackup tar with long name fails');
+	unlink "$superlongpath";
+}
 
 # The following tests are for symlinks.

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 6 Jul 2023 15:50:57 +0200

> On 5 Jul 2023, at 14:49, Andrew Dunstan <[email-masked]> wrote:

No objections to the patch, LGTM.

--
Daniel Gustafsson

---

From: Andrew Dunstan <[email-masked]>
Date: Thu, 6 Jul 2023 12:38:03 -0400

On 2023-07-06 Th 09:50, Daniel Gustafsson wrote:
>> On 5 Jul 2023, at 14:49, Andrew Dunstan<[email-masked]>  wrote:

Thanks. pushed with a couple of tweaks.

cheers

andrew

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 8 Jul 2023 09:15:21 -0400

> Thanks. pushed with a couple of tweaks.

Unfortunately, skipping this has now exposed a further problem in this test.

Here's the relevant log extracted from 
<https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren&dt=2023-07-07%2022%3A03%3A06>, 
starting with the skip mentioned above:

[23:29:21.661](0.002s) ok 98 # skip File path too long
### Stopping node "main" using mode fast
# Running: pg_ctl -D C:\\tools\\nmsys64\\home\\pgrunner\\bf\\root\\REL_16_STABLE\\pgsql.build/testrun/pg_basebackup/010_pg_basebackup/data/t_010_pg_basebackup_main_data/pgdata -m fast stop
waiting for server to shut down.... done
server stopped
# No postmaster PID for node "main"
Junction created for C:\\tools\\nmsys64\\home\\pgrunner\\bf\\root\\REL_16_STABLE\\pgsql.build\\testrun\\pg_basebackup\\010_pg_basebackup\\data\\t_010_pg_basebackup_main_data\\pgdata\\pg_replslot <<===>> C:\\tools\\nmsys64\\home\\pgrunner\\bf\\root\\REL_16_STABLE\\pgsql.build\\testrun\\pg_basebackup\\010_pg_basebackup\\data\\tmp_test_pjj2\\pg_replslot
### Starting node "main"
# Running: pg_ctl -w -D C:\\tools\\nmsys64\\home\\pgrunner\\bf\\root\\REL_16_STABLE\\pgsql.build/testrun/pg_basebackup/010_pg_basebackup/data/t_010_pg_basebackup_main_data/pgdata -l C:\\tools\\nmsys64\\home\\pgrunner\\bf\\root\\REL_16_STABLE\\pgsql.build/testrun/pg_basebackup/010_pg_basebackup/log/010_pg_basebackup_main.log -o --cluster-name=main start
waiting for server to start.... done
server started
# Postmaster PID for node "main" is 5184
Junction created for C:\\tools\\nmsys64\\tmp\\6zkMt003MF\\tempdir <<===>> C:\\tools\\nmsys64\\home\\pgrunner\\bf\\root\\REL_16_STABLE\\pgsql.build\\testrun\\pg_basebackup\\010_pg_basebackup\\data\\tmp_test_pjj2
# Taking pg_basebackup tarbackup2 from node "main"
# Running: pg_basebackup -D C:\\tools\\nmsys64\\home\\pgrunner\\bf\\root\\REL_16_STABLE\\pgsql.build/testrun/pg_basebackup/010_pg_basebackup/data/t_010_pg_basebackup_main_data/backup/tarbackup2 -h C:/tools/nmsys64/tmp/63ohSgsh21 -p 54699 --checkpoint fast --no-sync -Ft
WARNING:  aborting backup due to backend exiting before pg_backup_stop was called
pg_basebackup: error: could not initiate base backup: ERROR:  could not get junction for "./pg_replslot": More data is available.

It's worth pointing out that the path for the replslot junction is almost as long as the original path.

Since this test is passing on HEAD which has slightly shorter paths, I'm wondering if we should change this:

    rename("$pgdata/pg_replslot", "$tempdir/pg_replslot")
       or BAIL_OUT "could not move $pgdata/pg_replslot";
    dir_symlink("$tempdir/pg_replslot", "$pgdata/pg_replslot")
       or BAIL_OUT "could not symlink to $pgdata/pg_replslot";

to use the much shorter $sys_tempdir created a few lines below.

cheers

andrew

---

From: Andrew Dunstan <[email-masked]>
Date: Sat, 8 Jul 2023 11:52:11 -0400

> to use the much shorter $sys_tempdir created a few lines below.

Pushed a tested fix along those lines.

cheers

andrew

---

From: Alexander Lakhin <[email-masked]>
Date: Sat, 11 Nov 2023 16:00:00 +0300

Hello Andrew,

> Pushed a tested fix along those lines.

Today I've started up my Windows VM to run some tests and discovered a test
failure caused by that fix (e213de8e7):
 >meson test
Ok:                 246
Expected Fail:      0
Fail:               1
Unexpected Pass:    0
Skipped:            14
Timeout:            0

...\010_pg_basebackup\log\regress_log_010_pg_basebackup.txt contains:
[04:42:45.321](0.291s) Bail out!  could not move 
T:\postgresql\build/testrun/pg_basebackup/010_pg_basebackup\data/t_010_pg_basebackup_main_data/pgdata/pg_replslot

With a diagnostic print added before rename() in 010_pg_basebackup.pl, I see:
rename("T:\postgresql\build/testrun/pg_basebackup/010_pg_basebackup\data/t_010_pg_basebackup_main_data/pgdata/pg_replslot", 
"C:\Users\User\AppData\Local\Temp\fGT76tZUWr/pg_replslot")
That is, I have the postgres source tree and the user tempdir placed on
different disks.

perldoc on rename() says that it usually doesn't work across filesystem
boundaries, so I think it's not a Windows-specific issue.

Best regards,
Alexander

---

From: Alvaro Herrera <[email-masked]>
Date: Sat, 11 Nov 2023 17:31:39 +0100

> pg_basebackup: error: could not initiate base backup: ERROR:  could not get junction for "./pg_replslot": More data is available.

Why not patch pgreadlink to use the method recommended by Microsoft,
that DeviceIoControl() is called first with a NULL reparseBuffer to
determine the size needed, then a second time with a buffer of that
size?

https://learn.microsoft.com/en-us/windows/win32/api/ioapiset/nf-ioapiset-deviceiocontrol

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/