Re: fix tablespace handling in pg_combinebackup

2024-04-22 Thread Andres Freund
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

Re: fix tablespace handling in pg_combinebackup

2024-04-22 Thread Robert Haas
On Mon, Apr 22, 2024 at 5:16 PM Thomas Munro wrote: > On Tue, Apr 23, 2024 at 8:05 AM Robert Haas 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

Re: fix tablespace handling in pg_combinebackup

2024-04-22 Thread Andres Freund
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

Re: fix tablespace handling in pg_combinebackup

2024-04-22 Thread Thomas Munro
On Tue, Apr 23, 2024 at 8:05 AM Robert Haas 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

Re: fix tablespace handling in pg_combinebackup

2024-04-22 Thread Robert Haas
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 v1-0001-Try-again-to-add-test-coverage-for-pg_combineback.patch

Re: fix tablespace handling in pg_combinebackup

2024-04-22 Thread Alexander Lakhin
22.04.2024 00:49, Thomas Munro wrote: On Mon, Apr 22, 2024 at 12:00 AM Alexander Lakhin wrote: From what I can see, the following condition (namely, -l): if ($path =~ /^pg_tblspc\/(\d+)$/ && -l "$backup_path/$path") { push @tsoids,

Re: fix tablespace handling in pg_combinebackup

2024-04-21 Thread Thomas Munro
On Mon, Apr 22, 2024 at 12:00 AM Alexander Lakhin 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; >

Re: fix tablespace handling in pg_combinebackup

2024-04-21 Thread Alexander Lakhin
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")     {

Re: fix tablespace handling in pg_combinebackup

2024-04-19 Thread Thomas Munro
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

Re: fix tablespace handling in pg_combinebackup

2024-04-19 Thread Robert Haas
On Fri, Apr 19, 2024 at 4:18 PM Tom Lane wrote: > Robert Haas writes: > > On Fri, Apr 19, 2024 at 3:31 PM Tom Lane 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

Re: fix tablespace handling in pg_combinebackup

2024-04-19 Thread Tom Lane
Robert Haas writes: > On Fri, Apr 19, 2024 at 3:31 PM Tom Lane 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. >

Re: fix tablespace handling in pg_combinebackup

2024-04-19 Thread Robert Haas
On Fri, Apr 19, 2024 at 3:31 PM Tom Lane 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

Re: fix tablespace handling in pg_combinebackup

2024-04-19 Thread Tom Lane
Robert Haas writes: > On Fri, Apr 19, 2024 at 2:44 PM Tom Lane 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

Re: fix tablespace handling in pg_combinebackup

2024-04-19 Thread Robert Haas
On Fri, Apr 19, 2024 at 2:44 PM Tom Lane wrote: > Robert Haas writes: > > On Thu, Apr 18, 2024 at 1:45 PM Andres Freund 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

Re: fix tablespace handling in pg_combinebackup

2024-04-19 Thread Tom Lane
Robert Haas writes: > On Thu, Apr 18, 2024 at 1:45 PM Andres Freund 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

Re: fix tablespace handling in pg_combinebackup

2024-04-18 Thread Robert Haas
On Thu, Apr 18, 2024 at 1:45 PM Andres Freund 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

Re: fix tablespace handling in pg_combinebackup

2024-04-18 Thread Andres Freund
Hi, On 2024-04-18 09:03:21 -0400, Robert Haas wrote: > On Wed, Apr 17, 2024 at 5:50 PM Andres Freund 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

Re: fix tablespace handling in pg_combinebackup

2024-04-18 Thread Robert Haas
On Wed, Apr 17, 2024 at 5:50 PM Andres Freund 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 > >

Re: fix tablespace handling in pg_combinebackup

2024-04-18 Thread Michael Paquier
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

Re: fix tablespace handling in pg_combinebackup

2024-04-17 Thread Andres Freund
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

fix tablespace handling in pg_combinebackup

2024-04-17 Thread Robert Haas
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)