Обсуждение: fix tablespace handling in pg_combinebackup

In the "Differential code coverage between 16 and HEAD" thread, Andres
pointed out that there wasn't test case coverage for
pg_combinebackup's code to handle files in tablespaces. I looked at
adding that, and as nobody could possibly have predicted, found a bug.

Here's a 2-patch series to (1) enhance
PostgreSQL::Test::Utils::init_from_backup to handle tablespaces and
then (2) fix the bug in pg_combinebackup and add test coverage using
the facilities from the first patch.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2024-04-17 16:16:55 -0400, Robert Haas wrote:
> In the "Differential code coverage between 16 and HEAD" thread, Andres
> pointed out that there wasn't test case coverage for
> pg_combinebackup's code to handle files in tablespaces. I looked at
> adding that, and as nobody could possibly have predicted, found a bug.

Ha ;)


> @@ -787,8 +787,13 @@ Does not start the node after initializing it.
>  
>  By default, the backup is assumed to be plain format.  To restore from
>  a tar-format backup, pass the name of the tar program to use in the
> -keyword parameter tar_program.  Note that tablespace tar files aren't
> -handled here.
> +keyword parameter tar_program.
> +
> +If there are tablespace present in the backup, include tablespace_map as
> +a keyword parameter whose values is a hash. When tar_program is used, the
> +hash keys are tablespace OIDs; otherwise, they are the tablespace pathnames
> +used in the backup. In either case, the values are the tablespace pathnames
> +that should be used for the target cluster.

Where would one get these oids?


Could some of this be simplified by using allow_in_place_tablespaces instead?
Looks like it'd simplify at least the extended test somewhat?

Greetings,

Andres Freund

On Wed, Apr 17, 2024 at 02:50:21PM -0700, Andres Freund wrote:
> On 2024-04-17 16:16:55 -0400, Robert Haas wrote:
>> In the "Differential code coverage between 16 and HEAD" thread, Andres
>> pointed out that there wasn't test case coverage for
>> pg_combinebackup's code to handle files in tablespaces. I looked at
>> adding that, and as nobody could possibly have predicted, found a bug.
>
> Ha ;)

Note: open_item_counter++
--
Michael

On Wed, Apr 17, 2024 at 5:50 PM Andres Freund <andres@anarazel.de> wrote:
> > +If there are tablespace present in the backup, include tablespace_map as
> > +a keyword parameter whose values is a hash. When tar_program is used, the
> > +hash keys are tablespace OIDs; otherwise, they are the tablespace pathnames
> > +used in the backup. In either case, the values are the tablespace pathnames
> > +that should be used for the target cluster.
>
> Where would one get these oids?

You pretty much have to pick them out of the tar file names. It sucks,
but it's not this patch's fault. That's just how pg_basebackup works.
If you do a directory format backup, you can use -T to relocate
tablespaces on the fly, using the pathnames from the origin server.
That's a weird convention, and we probably should have based on the
tablespace names and not exposed the server pathnames to the client at
all, but we didn't. But it's still better than what happens when you
do a tar-format backup. In that case you just get a bunch of $OID.tar
files. No trace of the server pathnames remains, and the only way you
could learn the tablespace names is if you rooted through whatever
file contains the contents of the pg_tablespace system catalog. So
you've just got a bunch of OID-named things and it's all up to you to
figure out which one is which and what to put in the tablespace_map
file. I'd call this terrible UI design, but I think it's closer to
absence of UI design.

I wonder if we (as a project) would consider a patch that redesigned
this whole mechanism. Produce ${TABLESPACE_NAME}.tar in tar-format,
instead of ${OID}.tar. In directory-format, relocate via
-T${TABLESPACE_NAME}=${DIR} instead of -T${SERVERDIR}=${DIR}. That
would be a significant compatibility break, and you'd somehow need to
solve the problem of what to put in the tablespace_map file, which
requires OIDs. But it seems like if you could finesse that issue in
some elegant way, the result would just be a heck of a lot more usable
than what we have today.

> Could some of this be simplified by using allow_in_place_tablespaces instead?
> Looks like it'd simplify at least the extended test somewhat?

I don't think we can afford to assume that allow_in_place_tablespaces
doesn't change the behavior. I said (at least off-list) when that
feature was introduced that there was no way it was going to remain an
isolated development hack, and I think that's proved to be 100%
correct. We keep needing code to support it in more places, and I
expect that to continue. Probably we're going to need to start testing
everything both ways, which I think was a pretty predictable result of
introducing it in the first place.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2024-04-18 09:03:21 -0400, Robert Haas wrote:
> On Wed, Apr 17, 2024 at 5:50 PM Andres Freund <andres@anarazel.de> wrote:
> > > +If there are tablespace present in the backup, include tablespace_map as
> > > +a keyword parameter whose values is a hash. When tar_program is used, the
> > > +hash keys are tablespace OIDs; otherwise, they are the tablespace pathnames
> > > +used in the backup. In either case, the values are the tablespace pathnames
> > > +that should be used for the target cluster.
> >
> > Where would one get these oids?
> 
> You pretty much have to pick them out of the tar file names. It sucks,
> but it's not this patch's fault.

I was really just remarking on this from the angle of a test writer. I know
that our interfaces around this suck...

For tests, do we really need to set anything on a per-tablespace basis? Can't
we instead just reparent all of them to a new directory?


> I wonder if we (as a project) would consider a patch that redesigned
> this whole mechanism. Produce ${TABLESPACE_NAME}.tar in tar-format,
> instead of ${OID}.tar. In directory-format, relocate via
> -T${TABLESPACE_NAME}=${DIR} instead of -T${SERVERDIR}=${DIR}. That
> would be a significant compatibility break, and you'd somehow need to
> solve the problem of what to put in the tablespace_map file, which
> requires OIDs. But it seems like if you could finesse that issue in
> some elegant way, the result would just be a heck of a lot more usable
> than what we have today.

For some things that'd definitely be nicer - but not sure it work well for
everything. Consider cases where you actually have external directories on
different disks, and you want to restore a backup after some data loss. Now
you need to list all the tablespaces separately, to put them back into their
own location.

One thing I've been wondering about is an option to put the "new" tablespaces
into a location relative to each of the old ones.
  --tablespace-relative-location=../restore-2024-04-18
which would rewrite all the old tablespaces to that new location.


> > Could some of this be simplified by using allow_in_place_tablespaces instead?
> > Looks like it'd simplify at least the extended test somewhat?
> 
> I don't think we can afford to assume that allow_in_place_tablespaces
> doesn't change the behavior.

I think we can't assume that absolutely everywhere, but we don't need to test
it in a lot of places.


> I said (at least off-list) when that feature was introduced that there was
> no way it was going to remain an isolated development hack, and I think
> that's proved to be 100% correct.

Hm, I guess I kinda agree. But not because I think it wasn't good for
development, but because it'd often be much saner to use relative tablespaces
than absolute ones even for prod.

My only point here was that the test would be simpler if you
a) didn't need to create a temp directory for the tablespace, both for
   primary and standby
b) didn't need to "gin up" a tablespace map, because all the paths are
   relative


Just to be clear: I don't want the above to block merging your test. If you
think you want to do it the way you did, please do.

Greetings,

Andres Freund

On Thu, Apr 18, 2024 at 1:45 PM Andres Freund <andres@anarazel.de> wrote:
> I was really just remarking on this from the angle of a test writer. I know
> that our interfaces around this suck...
>
> For tests, do we really need to set anything on a per-tablespace basis? Can't
> we instead just reparent all of them to a new directory?

I don't know what you mean by this.

> > I wonder if we (as a project) would consider a patch that redesigned
> > this whole mechanism. Produce ${TABLESPACE_NAME}.tar in tar-format,
> > instead of ${OID}.tar. In directory-format, relocate via
> > -T${TABLESPACE_NAME}=${DIR} instead of -T${SERVERDIR}=${DIR}. That
> > would be a significant compatibility break, and you'd somehow need to
> > solve the problem of what to put in the tablespace_map file, which
> > requires OIDs. But it seems like if you could finesse that issue in
> > some elegant way, the result would just be a heck of a lot more usable
> > than what we have today.
>
> For some things that'd definitely be nicer - but not sure it work well for
> everything. Consider cases where you actually have external directories on
> different disks, and you want to restore a backup after some data loss. Now
> you need to list all the tablespaces separately, to put them back into their
> own location.

I mean, don't you need to do that anyway, just in a more awkward way?
I don't understand who ever wants to keep track of their tablespaces
by either (a) source pathname or (b) OID rather than (c) user-visible
tablespace name.

> One thing I've been wondering about is an option to put the "new" tablespaces
> into a location relative to each of the old ones.
>   --tablespace-relative-location=../restore-2024-04-18
> which would rewrite all the old tablespaces to that new location.

I think this would probably get limited use outside of testing scenarios.

> > I said (at least off-list) when that feature was introduced that there was
> > no way it was going to remain an isolated development hack, and I think
> > that's proved to be 100% correct.
>
> Hm, I guess I kinda agree. But not because I think it wasn't good for
> development, but because it'd often be much saner to use relative tablespaces
> than absolute ones even for prod.
>
> My only point here was that the test would be simpler if you
> a) didn't need to create a temp directory for the tablespace, both for
>    primary and standby
> b) didn't need to "gin up" a tablespace map, because all the paths are
>    relative
>
> Just to be clear: I don't want the above to block merging your test. If you
> think you want to do it the way you did, please do.

I think I will go ahead and do that soon, then. I'm totally fine with
it if somebody wants to do the testing in some other way, but this was
what made sense to me as the easiest way to adapt what's already
there. I think it's lame that init_from_backup() disclaims support for
tablespaces, as if tablespaces weren't a case that needs to be tested
heavily with respect to backups. And I think it's better to get
something merged that fixes the bug ASAP, and adds some test coverage,
and then if there's a better way to do that test coverage down the
road, well and good.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Apr 18, 2024 at 1:45 PM Andres Freund <andres@anarazel.de> wrote:
>> Just to be clear: I don't want the above to block merging your test. If you
>> think you want to do it the way you did, please do.

> I think I will go ahead and do that soon, then.

This patch failed to survive contact with the buildfarm.  It looks
like the animals that are unhappy are choking like this:

pg_basebackup: error: backup failed: ERROR:  symbolic link target too long for tar format: file name "pg_tblspc/16415",
target"/home/bf/bf-build/olingo/HEAD/pgsql.build/testrun/pg_combinebackup/002_compare_backups/data/tmp_test_bV72/ts" 

So whether it works depends on how long the path to the animal's build
root is.

This is not good at all, because if the buildfarm is hitting this
limit then actual users are likely to hit it as well.  But doesn't
POSIX define some way to get longer symlink paths into tar format?
(If not POSIX, I bet there's a widely-accepted GNU extension.)

            regards, tom lane

On Fri, Apr 19, 2024 at 2:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Thu, Apr 18, 2024 at 1:45 PM Andres Freund <andres@anarazel.de> wrote:
> >> Just to be clear: I don't want the above to block merging your test. If you
> >> think you want to do it the way you did, please do.
>
> > I think I will go ahead and do that soon, then.
>
> This patch failed to survive contact with the buildfarm.  It looks
> like the animals that are unhappy are choking like this:
>
> pg_basebackup: error: backup failed: ERROR:  symbolic link target too long for tar format: file name
"pg_tblspc/16415",target
"/home/bf/bf-build/olingo/HEAD/pgsql.build/testrun/pg_combinebackup/002_compare_backups/data/tmp_test_bV72/ts"
>
> So whether it works depends on how long the path to the animal's build
> root is.
>
> This is not good at all, because if the buildfarm is hitting this
> limit then actual users are likely to hit it as well.  But doesn't
> POSIX define some way to get longer symlink paths into tar format?
> (If not POSIX, I bet there's a widely-accepted GNU extension.)

Ah, crap. That sucks. As far as I've been able to find, we have no
code in the tree that knows how to generate symlinks longer than 99
characters (see tarCreateHeader). I can search around and see if I can
find something else out there on the Internet.

I feel like this is not a new problem but one I've had to dodge
before. In fact, I think I had to dodge it locally when developing
this patch. I believe that in various test cases, we rely on the fact
that PostgreSQL::Test::Utils::tempdir() produces pathnames that tend
to be shorter than the ones you get if you generate a path using
$node->backup_dir or $node->basedir or whatever. And I think if I
don't do it that way, it fails even locally on my machine. But, I
thought that were using that workaround elsewhere successfully, so I
expected it to be OK.

But I think in 010_pg_basebackup.pl we actually work harder to avoid
the problem than I had realized:

my $sys_tempdir = PostgreSQL::Test::Utils::tempdir_short;
...
# Test backup of a tablespace using tar format.
# Symlink the system located tempdir to our physical temp location.
# That way we can use shorter names for the tablespace directories,
# which hopefully won't run afoul of the 99 character length limit.
my $real_sys_tempdir = "$sys_tempdir/tempdir";
dir_symlink "$tempdir", $real_sys_tempdir;

mkdir "$tempdir/tblspc1";
my $realTsDir = "$real_sys_tempdir/tblspc1";

Maybe I need to clone that workaround here.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Apr 19, 2024 at 2:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This patch failed to survive contact with the buildfarm.  It looks
>> like the animals that are unhappy are choking like this:
>> pg_basebackup: error: backup failed: ERROR:  symbolic link target too long for tar format: file name
"pg_tblspc/16415",target
"/home/bf/bf-build/olingo/HEAD/pgsql.build/testrun/pg_combinebackup/002_compare_backups/data/tmp_test_bV72/ts"

> Ah, crap. That sucks. As far as I've been able to find, we have no
> code in the tree that knows how to generate symlinks longer than 99
> characters (see tarCreateHeader). I can search around and see if I can
> find something else out there on the Internet.

wikipedia has some useful info:

https://en.wikipedia.org/wiki/Tar_(computing)#POSIX.1-2001/pax

However, post-feature-freeze is not the time to be messing with
implementing pax.  Should we revert handling of tablespaces in this
program for now?

> But I think in 010_pg_basebackup.pl we actually work harder to avoid
> the problem than I had realized:
> ...
> Maybe I need to clone that workaround here.

That would be a reasonable answer if we deem the problem to be
just "the buildfarm is unhappy".  What I'm wondering about is
whether the feature will be useful to end users with this
pathname length restriction.

            regards, tom lane

On Fri, Apr 19, 2024 at 3:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> That would be a reasonable answer if we deem the problem to be
> just "the buildfarm is unhappy".  What I'm wondering about is
> whether the feature will be useful to end users with this
> pathname length restriction.

Possibly you're getting a little too enthusiastic about these revert
requests, because I'd say it's at least a decade too late to get rid
of pg_basebackup.

As discussed elsewhere, I do rather hope that pg_combinebackup will
eventually know how to operate on tar files as well, but right now it
doesn't.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Apr 19, 2024 at 3:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> That would be a reasonable answer if we deem the problem to be
>> just "the buildfarm is unhappy".  What I'm wondering about is
>> whether the feature will be useful to end users with this
>> pathname length restriction.

> Possibly you're getting a little too enthusiastic about these revert
> requests, because I'd say it's at least a decade too late to get rid
> of pg_basebackup.

I misunderstood the context then.  I thought you had just added
support for tablespaces in this area.  If pg_basebackup has been
choking on overly-long tablespace symlinks this whole time, then
the lack of field complaints suggests it's not such a common
case after all.

            regards, tom lane

On Fri, Apr 19, 2024 at 4:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Fri, Apr 19, 2024 at 3:31 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> That would be a reasonable answer if we deem the problem to be
> >> just "the buildfarm is unhappy".  What I'm wondering about is
> >> whether the feature will be useful to end users with this
> >> pathname length restriction.
>
> > Possibly you're getting a little too enthusiastic about these revert
> > requests, because I'd say it's at least a decade too late to get rid
> > of pg_basebackup.
>
> I misunderstood the context then.  I thought you had just added
> support for tablespaces in this area.  If pg_basebackup has been
> choking on overly-long tablespace symlinks this whole time, then
> the lack of field complaints suggests it's not such a common
> case after all.

No, the commit that caused all this was a 1-line code change. It was a
pretty stupid mistake which I would have avoided if I'd had proper
test case coverage for it, but I didn't do that originally. I think my
underlying reason for not doing the work was that I feared it would be
hard to test in a way that was stable. But, the existence of a bug
obviously proved that the test cases were needed. As expected,
however, that was hard to do without breaking things.

If you look at the error message you sent me, you can see that while
it's a pg_combinebackup test that is failing, the actual failing
program is pg_basebackup.

--
Robert Haas
EDB: http://www.enterprisedb.com

I don't know how to fix 82023d47^ on Windows[1][2], but in case it's
useful, here's a small clue.  I see that Perl's readlink() does in
fact know how to read "junction points" on Windows[3], so if that was
the only problem it might have been very close to working.  One
difference is that our own src/port/dirmod.c's pgreadlink() also
strips \??\ from the returned paths (something to do with the
various forms of NT path), but when I tried that:

                        my $olddir = readlink("$backup_path/pg_tblspc/$tsoid")
                                || die "readlink
$backup_path/pg_tblspc/$tsoid: $!";

+                       # strip NT path prefix (see src/port/dirmod.c
pgreadlink())
+                       $olddir =~ s/^\\\?\?\\// if
$PostgreSQL::Test::Utils::windows_os;

... it still broke[4].  So I'm not sure what's going on...

[1] https://github.com/postgres/postgres/runs/24040897199
[2]
https://api.cirrus-ci.com/v1/artifact/task/5550091866472448/testrun/build/testrun/pg_combinebackup/002_compare_backups/log/002_compare_backups_pitr1.log
[3] https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L2041
[4] https://cirrus-ci.com/task/6746621638082560

Hello Thomas and Robert,

20.04.2024 08:56, Thomas Munro wrote:
> ... it still broke[4].  So I'm not sure what's going on...
>

 From what I can see, the following condition (namely, -l):
                 if ($path =~ /^pg_tblspc\/(\d+)$/ && -l "$backup_path/$path")
                 {
                     push @tsoids, $1;
                     return 0;
                 }

is false for junction points on Windows (cf [1]), but the target path is:
  Directory of 

C:\src\postgresql\build\testrun\pg_combinebackup\002_compare_backups\data\t_002_compare_backups_primary_data\backup\backup1\pg_tblspc

04/21/2024  02:05 PM    <DIR>          .
04/21/2024  02:05 PM    <DIR>          ..
04/21/2024  02:05 PM    <JUNCTION>     16415 [\??\C:\Users\ADMINI~1\AppData\Local\Temp\xXMfNDMCot\ts1backup]

[1] https://www.perlmonks.org/?node_id=1223819

Best regards,
Alexander

On Mon, Apr 22, 2024 at 12:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>  From what I can see, the following condition (namely, -l):
>                  if ($path =~ /^pg_tblspc\/(\d+)$/ && -l "$backup_path/$path")
>                  {
>                      push @tsoids, $1;
>                      return 0;
>                  }
>
> is false for junction points on Windows (cf [1]), but the target path is:

Ah, yes, right, -l doesn't like junction points.  Well, we're already
using the Win32API::File package (it looks like a CPAN library, but I
guess the Windows perl distros like Strawberry are all including it
for free?).  See PostgreSQL::Test::Utils::is_symlink(), attached.
That seems to work as expected, but someone who actually knows perl
can surely make it better.  Then I hit the next problem:

readlink
C:\cirrus\build/testrun/pg_combinebackup/002_compare_backups\data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/16415:
Inappropriate I/O control operation at
C:/cirrus/src/test/perl/PostgreSQL/Test/Cluster.pm line 927.

https://cirrus-ci.com/task/5162332353986560

I don't know where exactly that error message is coming from, but
assuming that Strawberry Perl contains this code:

https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L2041
https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L1976

... then it's *very* similar to what we're doing in our own
pgreadlink() code.  I wondered if the buffer size might be too small
for our path, but it doesn't seem so:

https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L1581C1-L1581C35

(I think MAX_PATH is 256 on Windows.)

If there is some unfixable problem with what they're doing in their
readlink(), then I guess it should be possible to read the junction
point directly in Perl using Win32API::File::DeviceIoControl()... but
I can't see what's wrong with it!  Maybe it's not the same code?

Attached are the new test support functions, and the fixup to Robert's
6bf5c42b that uses them.  To be clear, this doesn't work, yet.  It has
got to be close though...

22.04.2024 00:49, Thomas Munro wrote:
> On Mon, Apr 22, 2024 at 12:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>>   From what I can see, the following condition (namely, -l):
>>                   if ($path =~ /^pg_tblspc\/(\d+)$/ && -l "$backup_path/$path")
>>                   {
>>                       push @tsoids, $1;
>>                       return 0;
>>                   }
>>
>> is false for junction points on Windows (cf [1]), but the target path is:
> Ah, yes, right, -l doesn't like junction points.  Well, we're already
> using the Win32API::File package (it looks like a CPAN library, but I
> guess the Windows perl distros like Strawberry are all including it
> for free?).  See PostgreSQL::Test::Utils::is_symlink(), attached.
> That seems to work as expected, but someone who actually knows perl
> can surely make it better.  Then I hit the next problem:
>
> readlink
C:\cirrus\build/testrun/pg_combinebackup/002_compare_backups\data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/16415:
> Inappropriate I/O control operation at
> C:/cirrus/src/test/perl/PostgreSQL/Test/Cluster.pm line 927.
>
> https://cirrus-ci.com/task/5162332353986560
>
> I don't know where exactly that error message is coming from, but
> assuming that Strawberry Perl contains this code:
>
> https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L2041
> https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L1976
>
> ... then it's *very* similar to what we're doing in our own
> pgreadlink() code.  I wondered if the buffer size might be too small
> for our path, but it doesn't seem so:
>
> https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L1581C1-L1581C35
>
> (I think MAX_PATH is 256 on Windows.)
>
> If there is some unfixable problem with what they're doing in their
> readlink(), then I guess it should be possible to read the junction
> point directly in Perl using Win32API::File::DeviceIoControl()... but
> I can't see what's wrong with it!  Maybe it's not the same code?

I wonder whether the target path (\??\) of that junction point is fully correct.
I tried:
 > mklink /j 

"C:/src/postgresql/build/testrun/pg_combinebackup/002_compare_backups/data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/test"

\\?\C:\t1
Junction created for 

C:/src/postgresql/build/testrun/pg_combinebackup/002_compare_backups/data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/test

<<===>> \\?\C:\t1
and
my $path = 

'C:/src/postgresql/build/testrun/pg_combinebackup/002_compare_backups/data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/test';
my $result = readlink($path);
works for me:
result: \\?\C:\t1

Whilst with:
my $path = 

'C:/src/postgresql/build/testrun/pg_combinebackup/002_compare_backups/data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/16415';
readlink() fails with "Invalid argument".

 > dir 

"C:/src/postgresql/build/testrun/pg_combinebackup/002_compare_backups/data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc"
04/22/2024  08:16 AM    <DIR>          .
04/22/2024  08:16 AM    <DIR>          ..
04/22/2024  06:52 AM    <JUNCTION>     16415 [\??\C:\Users\ADMINI~1\AppData\Local\Temp\1zznr8FW5N\ts1backup]
04/22/2024  08:16 AM    <JUNCTION>     test [\\?\C:\t1]

 > dir "\??\C:\Users\ADMINI~1\AppData\Local\Temp\1zznr8FW5N\ts1backup"
  Directory of C:\??\C:\Users\ADMINI~1\AppData\Local\Temp\1zznr8FW5N\ts1backup

File Not Found

 > dir "\\?\C:\t1"
  Directory of \\?\C:\t1

04/22/2024  08:06 AM    <DIR>          .
04/22/2024  08:06 AM    <DIR>          ..
                0 File(s)              0 bytes

Though
 > dir 

"C:/src/postgresql/build/testrun/pg_combinebackup/002_compare_backups/data/t_002_compare_backups_primary_data/backup/backup1/pg_tblspc/16415"
somehow really works:
  Directory of 

C:\src\postgresql\build\testrun\pg_combinebackup\002_compare_backups\data\t_002_compare_backups_primary_data\backup\backup1\pg_tblspc\16415

04/22/2024  06:52 AM    <DIR>          .
04/22/2024  06:52 AM    <DIR>          ..
04/22/2024  06:52 AM    <DIR>          PG_17_202404021

Best regards,
Alexander

I reworked the test cases so that they don't (I think) rely on
symlinks working as they do on normal platforms.

Here's a patch. I'll go create a CommitFest entry for this thread so
that cfbot will look at it.

...Robert

On Tue, Apr 23, 2024 at 8:05 AM Robert Haas <robertmhaas@gmail.com> wrote:
> I reworked the test cases so that they don't (I think) rely on
> symlinks working as they do on normal platforms.

Cool.

(It will remain a mystery for now why perl readlink() can't read the
junction points that PostgreSQL creates (IIUC), but the OS can follow
them and PostgreSQL itself can read them with apparently similar code.
I find myself wondering if symlinks should go on the list of "things
we pretended Windows had out of convenience, that turned out to be
more inconvenient than we expected, and we'd do better to tackle
head-on with a more portable idea".  Perhaps we could just use a
tablespace map file instead to do our own path construction, or
something like that.  I suspect that would be one of those changes
that is technically easy, but community-wise hard as it affects a
load of backup tools and procedures...)

Hi,

On 2024-04-23 09:15:27 +1200, Thomas Munro wrote:
> I find myself wondering if symlinks should go on the list of "things
> we pretended Windows had out of convenience, that turned out to be
> more inconvenient than we expected, and we'd do better to tackle
> head-on with a more portable idea".

Yes, I think the symlink design was pretty clearly a mistake.


> Perhaps we could just use a tablespace map file instead to do our own path
> construction, or something like that.  I suspect that would be one of those
> changes that is technically easy, but community-wise hard as it affects a
> load of backup tools and procedures...)

Yea. I wonder if we could do a somewhat transparent upgrade by creating a file
alongside each tablespace that contains the path or such.

Greetings,

Andres

On Mon, Apr 22, 2024 at 5:16 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> On Tue, Apr 23, 2024 at 8:05 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > I reworked the test cases so that they don't (I think) rely on
> > symlinks working as they do on normal platforms.
>
> Cool.

cfbot is giving me a bunch of green check marks, so I plan to commit
this version, barring objections.

I shall leave redesign of the symlink mess as a matter for others to
ponder; I'm not in love with what we have, but I think it will be
tricky to do better, and I don't think I want to spend time on it, at
least not now.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2024-04-22 18:10:17 -0400, Robert Haas wrote:
> cfbot is giving me a bunch of green check marks, so I plan to commit
> this version, barring objections.

Makes sense.


> I shall leave redesign of the symlink mess as a matter for others to
> ponder; I'm not in love with what we have, but I think it will be
> tricky to do better, and I don't think I want to spend time on it, at
> least not now.

Oh, yea, that's clearly a much bigger and separate project...

Greetings,

Andres Freund