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
Attachments:
0001-Make-PostgreSQL-Test-Cluster-init_from_backup-handle.patchapplication/octet-stream; name=0001-Make-PostgreSQL-Test-Cluster-init_from_backup-handle.patchDownload
From 21cd35e8850bc64c9d8d65e9863aa5c31b26c215 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Wed, 17 Apr 2024 15:56:33 -0400
Subject: [PATCH 1/2] Make PostgreSQL::Test::Cluster::init_from_backup handle
tablespaces.
This commit doesn't use this infrastructure for anything new, although
it does adapt 010_pg_basebackup.pl to use it. However, a future commit
will use this to improve test coverage for pg_combinebackup.
---
src/bin/pg_basebackup/t/010_pg_basebackup.pl | 21 +----
src/test/perl/PostgreSQL/Test/Cluster.pm | 93 ++++++++++++++++++--
2 files changed, 90 insertions(+), 24 deletions(-)
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 63f7bd2735..31c369688e 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -407,25 +407,12 @@ SKIP:
my $node2 = PostgreSQL::Test::Cluster->new('replica');
- # Recover main data directory
- $node2->init_from_backup($node, 'tarbackup2', tar_program => $tar);
-
- # Recover tablespace into a new directory (not where it was!)
- my $repTsDir = "$tempdir/tblspc1replica";
- my $realRepTsDir = "$real_sys_tempdir/tblspc1replica";
- mkdir $repTsDir;
- PostgreSQL::Test::Utils::system_or_bail($tar, 'xf', $tblspc_tars[0],
- '-C', $repTsDir);
-
- # Update tablespace map to point to new directory.
- # XXX Ideally pg_basebackup would handle this.
+ # Recover the backup
$tblspc_tars[0] =~ m|/([0-9]*)\.tar$|;
my $tblspcoid = $1;
- my $escapedRepTsDir = $realRepTsDir;
- $escapedRepTsDir =~ s/\\/\\\\/g;
- open my $mapfile, '>', $node2->data_dir . '/tablespace_map' or die $!;
- print $mapfile "$tblspcoid $escapedRepTsDir\n";
- close $mapfile;
+ my $realRepTsDir = "$real_sys_tempdir/tblspc1replica";
+ $node2->init_from_backup($node, 'tarbackup2', tar_program => $tar,
+ 'tablespace_map' => { $tblspcoid => $realRepTsDir });
$node2->start;
my $result = $node2->safe_psql('postgres', 'SELECT * FROM test1');
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 9b2879c145..52972a9746 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -777,7 +777,7 @@ sub backup_fs_cold
=pod
-=item $node->init_from_backup(root_node, backup_name)
+=item $node->init_from_backup(root_node, backup_name, %params)
Initialize a node from a backup, which may come from this node or a different
node. root_node must be a PostgreSQL::Test::Cluster reference, backup_name the string name
@@ -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.
To restore from an incremental backup, pass the parameter combine_with_prior
as a reference to an array of prior backup names with which this backup
@@ -843,12 +848,20 @@ sub init_from_backup
}
local %ENV = $self->_get_env();
- PostgreSQL::Test::Utils::system_or_bail('pg_combinebackup', '-d',
- @prior_backup_path, $backup_path, '-o', $data_path);
+ my @combineargs = ('pg_combinebackup', '-d');
+ if (exists $params{tablespace_map})
+ {
+ while (my ($olddir, $newdir) = each %{$params{tablespace_map}})
+ {
+ push @combineargs, "-T$olddir=$newdir";
+ }
+ }
+ push @combineargs, @prior_backup_path, $backup_path, '-o', $data_path;
+ PostgreSQL::Test::Utils::system_or_bail(@combineargs);
}
elsif (defined $params{tar_program})
{
- mkdir($data_path);
+ mkdir($data_path) || die "mkdir $data_path: $!";
PostgreSQL::Test::Utils::system_or_bail($params{tar_program}, 'xf',
$backup_path . '/base.tar',
'-C', $data_path);
@@ -856,11 +869,77 @@ sub init_from_backup
$params{tar_program}, 'xf',
$backup_path . '/pg_wal.tar', '-C',
$data_path . '/pg_wal');
+
+ # We need to generate a tablespace_map file.
+ open(my $tsmap, ">", "$data_path/tablespace_map")
+ || die "$data_path/tablespace_map: $!";
+
+ # Extract tarfiles and add tablespace_map entries
+ my @tstars = grep { /^\d+.tar/ }
+ PostgreSQL::Test::Utils::slurp_dir($backup_path);
+ for my $tstar (@tstars)
+ {
+ my $tsoid = $tstar;
+ $tsoid =~ s/\.tar$//;
+
+ die "no tablespace mapping for $tstar"
+ if !exists $params{tablespace_map} ||
+ !exists $params{tablespace_map}{$tsoid};
+ my $newdir = $params{tablespace_map}{$tsoid};
+
+ mkdir($newdir) || die "mkdir $newdir: $!";
+ PostgreSQL::Test::Utils::system_or_bail($params{tar_program}, 'xf',
+ $backup_path . '/' . $tstar, '-C', $newdir);
+
+ my $escaped_newdir = $newdir;
+ $escaped_newdir =~ s/\\/\\\\/g;
+ print $tsmap "$tsoid $escaped_newdir\n";
+ }
+
+ # Close tablespace_map.
+ close($tsmap);
}
else
{
+ my @tsoids;
rmdir($data_path);
- PostgreSQL::Test::RecursiveCopy::copypath($backup_path, $data_path);
+
+ # Copy the main backup. Exclude tablespace links, but remember them.
+ PostgreSQL::Test::RecursiveCopy::copypath($backup_path, $data_path,
+ 'filterfn' => sub {
+ my ($path) = @_;
+ if ($path =~ /^pg_tblspc\/(\d+)$/ && -l "$backup_path/$path")
+ {
+ push @tsoids, $1;
+ return 0;
+ }
+ return 1;
+ });
+
+ # We need to generate a tablespace_map file.
+ open(my $tsmap, ">", "$data_path/tablespace_map")
+ || die "$data_path/tablespace_map: $!";
+
+ # Now use the list of tablespace links to copy each tablespace.
+ for my $tsoid (@tsoids)
+ {
+ my $olddir = readlink("$backup_path/pg_tblspc/$tsoid")
+ || die "readlink $backup_path/pg_tblspc/$tsoid: $!";
+
+ die "no tablespace mapping for $olddir"
+ if !exists $params{tablespace_map} ||
+ !exists $params{tablespace_map}{$olddir};
+
+ my $newdir = $params{tablespace_map}{$olddir};
+ PostgreSQL::Test::RecursiveCopy::copypath($olddir, $newdir);
+
+ my $escaped_newdir = $newdir;
+ $escaped_newdir =~ s/\\/\\\\/g;
+ print $tsmap "$tsoid $escaped_newdir\n";
+ }
+
+ # Close tablespace_map.
+ close($tsmap);
}
chmod(0700, $data_path) or die $!;
--
2.39.3 (Apple Git-145)
0002-pg_combinebackup-Fix-incorrect-tablespace-handling.patchapplication/octet-stream; name=0002-pg_combinebackup-Fix-incorrect-tablespace-handling.patchDownload
From 28f467db842ff0d114b5c68d201edf08d513667b Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Wed, 17 Apr 2024 16:10:20 -0400
Subject: [PATCH 2/2] pg_combinebackup: Fix incorrect tablespace handling.
The previous coding mangled the pathname calculation for
incremental files located in user-defined tablespaces.
Enhance the test cases to cover such cases, as I should have
done originally. Thanks to Andres Freund for alerting me to the
lack of test coverage.
---
src/bin/pg_combinebackup/pg_combinebackup.c | 2 +-
src/bin/pg_combinebackup/reconstruct.c | 7 ++--
.../pg_combinebackup/t/002_compare_backups.pl | 32 ++++++++++++++++---
3 files changed, 33 insertions(+), 8 deletions(-)
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index 4b23c47c95..2ee5014c2d 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -989,7 +989,7 @@ process_directory_recursively(Oid tsoid,
/* Reconstruction logic will do the rest. */
reconstruct_from_incremental_file(ifullpath, ofullpath,
- relative_path,
+ manifest_prefix,
de->d_name + INCREMENTAL_PREFIX_LENGTH,
n_prior_backups,
prior_backup_dirs,
diff --git a/src/bin/pg_combinebackup/reconstruct.c b/src/bin/pg_combinebackup/reconstruct.c
index 15f62c18df..04bd0e99da 100644
--- a/src/bin/pg_combinebackup/reconstruct.c
+++ b/src/bin/pg_combinebackup/reconstruct.c
@@ -75,9 +75,10 @@ static void read_block(rfile *s, off_t off, uint8 *buffer);
* output_filename should be the path where the reconstructed file is to be
* written.
*
- * relative_path should be the relative path to the directory containing this
- * file. bare_file_name should be the name of the file within that directory,
- * without "INCREMENTAL.".
+ * relative_path should be the path to the directory containing this file,
+ * relative to the root of the backup (NOT relative to the root of the
+ * tablespace). bare_file_name should be the name of the file within that
+ * directory, without "INCREMENTAL.".
*
* n_prior_backups is the number of prior backups, and prior_backup_dirs is
* an array of pathnames where those backups can be found.
diff --git a/src/bin/pg_combinebackup/t/002_compare_backups.pl b/src/bin/pg_combinebackup/t/002_compare_backups.pl
index 71e9a90d22..1d9764eeac 100644
--- a/src/bin/pg_combinebackup/t/002_compare_backups.pl
+++ b/src/bin/pg_combinebackup/t/002_compare_backups.pl
@@ -7,11 +7,15 @@ use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
+my $tempdir = PostgreSQL::Test::Utils::tempdir;
+
# Set up a new database instance.
my $primary = PostgreSQL::Test::Cluster->new('primary');
$primary->init(has_archiving => 1, allows_streaming => 1);
$primary->append_conf('postgresql.conf', 'summarize_wal = on');
$primary->start;
+my $tsprimary = $tempdir . '/ts';
+mkdir($tsprimary) || die "mkdir $tsprimary: $!";
# Create some test tables, each containing one row of data, plus a whole
# extra database.
@@ -29,24 +33,37 @@ INSERT INTO will_get_dropped VALUES (1, 'initial test row');
CREATE TABLE will_get_rewritten (a int, b text);
INSERT INTO will_get_rewritten VALUES (1, 'initial test row');
CREATE DATABASE db_will_get_dropped;
+CREATE TABLESPACE ts1 LOCATION '$tsprimary';
+CREATE TABLE will_not_change_in_ts (a int, b text) TABLESPACE ts1;
+INSERT INTO will_not_change_in_ts VALUES (1, 'initial test row');
+CREATE TABLE will_change_in_ts (a int, b text) TABLESPACE ts1;
+INSERT INTO will_change_in_ts VALUES (1, 'initial test row');
+CREATE TABLE will_get_dropped_in_ts (a int, b text);
+INSERT INTO will_get_dropped_in_ts VALUES (1, 'initial test row');
EOM
# Take a full backup.
my $backup1path = $primary->backup_dir . '/backup1';
+my $tsbackup1path = $tempdir . '/ts1backup';
+mkdir($tsbackup1path) || die "mkdir $tsbackup1path: $!";
$primary->command_ok(
- [ 'pg_basebackup', '-D', $backup1path, '--no-sync', '-cfast' ],
- "full backup");
+ [ 'pg_basebackup', '-D', $backup1path, '--no-sync', '-cfast',
+ "-T${tsprimary}=${tsbackup1path}" ], "full backup");
# Now make some database changes.
$primary->safe_psql('postgres', <<EOM);
UPDATE will_change SET b = 'modified value' WHERE a = 1;
+UPDATE will_change_in_ts SET b = 'modified value' WHERE a = 1;
INSERT INTO will_grow
SELECT g, 'additional row' FROM generate_series(2, 5000) g;
TRUNCATE will_shrink;
VACUUM will_get_vacuumed;
DROP TABLE will_get_dropped;
+DROP TABLE will_get_dropped_in_ts;
CREATE TABLE newly_created (a int, b text);
INSERT INTO newly_created VALUES (1, 'row for new table');
+CREATE TABLE newly_created_in_ts (a int, b text) TABLESPACE ts1;
+INSERT INTO newly_created_in_ts VALUES (1, 'row for new table');
VACUUM FULL will_get_rewritten;
DROP DATABASE db_will_get_dropped;
CREATE DATABASE db_newly_created;
@@ -54,8 +71,11 @@ EOM
# Take an incremental backup.
my $backup2path = $primary->backup_dir . '/backup2';
+my $tsbackup2path = $tempdir . '/tsbackup2';
+mkdir($tsbackup2path) || die "mkdir $tsbackup2path: $!";
$primary->command_ok(
[ 'pg_basebackup', '-D', $backup2path, '--no-sync', '-cfast',
+ "-T${tsprimary}=${tsbackup2path}",
'--incremental', $backup1path . '/backup_manifest' ],
"incremental backup");
@@ -78,9 +98,11 @@ $primary->poll_query_until('postgres', $archive_wait_query)
# Perform PITR from the full backup. Disable archive_mode so that the archive
# doesn't find out about the new timeline; that way, the later PITR below will
# choose the same timeline.
+my $tspitr1path = $tempdir . '/tspitr1';
my $pitr1 = PostgreSQL::Test::Cluster->new('pitr1');
$pitr1->init_from_backup($primary, 'backup1',
- standby => 1, has_restoring => 1);
+ standby => 1, has_restoring => 1,
+ tablespace_map => { $tsbackup1path => $tspitr1path });
$pitr1->append_conf('postgresql.conf', qq{
recovery_target_lsn = '$lsn'
recovery_target_action = 'promote'
@@ -90,10 +112,12 @@ $pitr1->start();
# Perform PITR to the same LSN from the incremental backup. Use the same
# basic configuration as before.
+my $tspitr2path = $tempdir . '/tspitr2';
my $pitr2 = PostgreSQL::Test::Cluster->new('pitr2');
$pitr2->init_from_backup($primary, 'backup2',
standby => 1, has_restoring => 1,
- combine_with_prior => [ 'backup1' ]);
+ combine_with_prior => [ 'backup1' ],
+ tablespace_map => { $tsbackup2path => $tspitr2path });
$pitr2->append_conf('postgresql.conf', qq{
recovery_target_lsn = '$lsn'
recovery_target_action = 'promote'
--
2.39.3 (Apple Git-145)
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
relativeJust 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]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, 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]https://github.com/Perl/perl5/blob/f936cd91ee430786a1bb6068a4a7c8362610dd5f/win32/win32.c#L2041, 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]https://cirrus-ci.com/task/6746621638082560. 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]https://www.perlmonks.org/?node_id=1223819), 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:
(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...
Attachments:
0001-More-Windows-pseudo-symlink-support-for-Perl-code.patchapplication/octet-stream; name=0001-More-Windows-pseudo-symlink-support-for-Perl-code.patchDownload
From 87e75dbff60d4cca96aaedf7f898a54e728874ec Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Sat, 20 Apr 2024 17:18:46 +1200
Subject: [PATCH 1/2] More Windows pseudo-symlink support for Perl code.
We already had PostgreSQL::Test::Utils::dir_symlink() to make junction
points, which we used instead of symlinks on Windows. Add is_symlink()
and read_symlink() functions, for use by TAP tests.
FIXME I don't know how to access the name FILE_ATTRIBUTE_REPARSE_POINT
from Win32API::File without getting weird "barename" warnings, because I
don't know perl, so I just looked up the numerical value, which works.
---
src/test/perl/PostgreSQL/Test/Utils.pm | 46 +++++++++++++++++++++++++-
1 file changed, 45 insertions(+), 1 deletion(-)
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 022b44ba22..5e3dfc8f1b 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -71,6 +71,8 @@ our @EXPORT = qw(
chmod_recursive
check_pg_config
dir_symlink
+ is_symlink
+ read_symlink
scan_server_header
system_or_bail
system_log
@@ -155,7 +157,7 @@ BEGIN
if ($windows_os)
{
require Win32API::File;
- Win32API::File->import(qw(createFile OsFHandleOpen CloseHandle));
+ Win32API::File->import(qw(createFile OsFHandleOpen CloseHandle GetFileAttributes));
}
# Specifies whether to use Unix sockets for test setups. On
@@ -805,6 +807,48 @@ sub dir_symlink
=pod
+=item is_symlink(path)
+
+Portably test if a path is a symlink. On Windows this tests for a junction
+point.
+
+=cut
+
+sub is_symlink
+{
+ my $path = shift;
+ if ($windows_os)
+ {
+ return GetFileAttributes($path) & 1024; # FILE_ATTRIBUTE_REPARSE_POINT
+ }
+ else
+ {
+ return -l $path;
+ }
+}
+
+=pod
+
+=item read_symlink(path)
+
+Portably read a symlink. On Windows, perl's readlink() already knows how to
+read junction points. To match dirmod.c's pgreadlink(), we strip the prefix
+from the full NT path and return just the "drive absolute" part.
+
+=cut
+
+sub read_symlink
+{
+ my $path = shift;
+ my $result = readlink($path);
+
+ $result =~ s/^\\\?\?\\// if $windows_os;
+
+ return $result;
+}
+
+=pod
+
=back
=head1 Test::More-LIKE METHODS
--
2.39.3 (Apple Git-146)
0002-fixup.patchapplication/octet-stream; name=0002-fixup.patchDownload
From 1d0b49a5ec3ddc40e345ef5bf2387cc6cf3683ac Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Mon, 22 Apr 2024 09:07:58 +1200
Subject: [PATCH 2/2] fixup
---
src/test/perl/PostgreSQL/Test/Cluster.pm | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 52972a9746..f87725f686 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -908,7 +908,8 @@ sub init_from_backup
PostgreSQL::Test::RecursiveCopy::copypath($backup_path, $data_path,
'filterfn' => sub {
my ($path) = @_;
- if ($path =~ /^pg_tblspc\/(\d+)$/ && -l "$backup_path/$path")
+ if ($path =~ /^pg_tblspc\/(\d+)$/ &&
+ PostgreSQL::Test::Utils::is_symlink("$backup_path/$path"))
{
push @tsoids, $1;
return 0;
@@ -923,7 +924,7 @@ sub init_from_backup
# Now use the list of tablespace links to copy each tablespace.
for my $tsoid (@tsoids)
{
- my $olddir = readlink("$backup_path/pg_tblspc/$tsoid")
+ my $olddir = PostgreSQL::Test::Utils::read_symlink("$backup_path/pg_tblspc/$tsoid")
|| die "readlink $backup_path/pg_tblspc/$tsoid: $!";
die "no tablespace mapping for $olddir"
--
2.39.3 (Apple Git-146)
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:(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
Attachments:
v1-0001-Try-again-to-add-test-coverage-for-pg_combineback.patchapplication/octet-stream; name=v1-0001-Try-again-to-add-test-coverage-for-pg_combineback.patchDownload
From 029fc34ce9f27aacb471a0f2164643a34bdbb757 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Mon, 22 Apr 2024 16:02:34 -0400
Subject: [PATCH v1] Try again to add test coverage for pg_combinebackup
w/tablespaces.
---
src/bin/pg_basebackup/t/010_pg_basebackup.pl | 21 +---
.../pg_combinebackup/t/002_compare_backups.pl | 37 ++++++-
src/test/perl/PostgreSQL/Test/Cluster.pm | 96 +++++++++++++++++--
3 files changed, 126 insertions(+), 28 deletions(-)
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 63f7bd2735..31c369688e 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -407,25 +407,12 @@ SKIP:
my $node2 = PostgreSQL::Test::Cluster->new('replica');
- # Recover main data directory
- $node2->init_from_backup($node, 'tarbackup2', tar_program => $tar);
-
- # Recover tablespace into a new directory (not where it was!)
- my $repTsDir = "$tempdir/tblspc1replica";
- my $realRepTsDir = "$real_sys_tempdir/tblspc1replica";
- mkdir $repTsDir;
- PostgreSQL::Test::Utils::system_or_bail($tar, 'xf', $tblspc_tars[0],
- '-C', $repTsDir);
-
- # Update tablespace map to point to new directory.
- # XXX Ideally pg_basebackup would handle this.
+ # Recover the backup
$tblspc_tars[0] =~ m|/([0-9]*)\.tar$|;
my $tblspcoid = $1;
- my $escapedRepTsDir = $realRepTsDir;
- $escapedRepTsDir =~ s/\\/\\\\/g;
- open my $mapfile, '>', $node2->data_dir . '/tablespace_map' or die $!;
- print $mapfile "$tblspcoid $escapedRepTsDir\n";
- close $mapfile;
+ my $realRepTsDir = "$real_sys_tempdir/tblspc1replica";
+ $node2->init_from_backup($node, 'tarbackup2', tar_program => $tar,
+ 'tablespace_map' => { $tblspcoid => $realRepTsDir });
$node2->start;
my $result = $node2->safe_psql('postgres', 'SELECT * FROM test1');
diff --git a/src/bin/pg_combinebackup/t/002_compare_backups.pl b/src/bin/pg_combinebackup/t/002_compare_backups.pl
index 71e9a90d22..31342579cf 100644
--- a/src/bin/pg_combinebackup/t/002_compare_backups.pl
+++ b/src/bin/pg_combinebackup/t/002_compare_backups.pl
@@ -7,11 +7,15 @@ use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
+my $tempdir = PostgreSQL::Test::Utils::tempdir_short();
+
# Set up a new database instance.
my $primary = PostgreSQL::Test::Cluster->new('primary');
$primary->init(has_archiving => 1, allows_streaming => 1);
$primary->append_conf('postgresql.conf', 'summarize_wal = on');
$primary->start;
+my $tsprimary = $tempdir . '/ts';
+mkdir($tsprimary) || die "mkdir $tsprimary: $!";
# Create some test tables, each containing one row of data, plus a whole
# extra database.
@@ -29,24 +33,42 @@ INSERT INTO will_get_dropped VALUES (1, 'initial test row');
CREATE TABLE will_get_rewritten (a int, b text);
INSERT INTO will_get_rewritten VALUES (1, 'initial test row');
CREATE DATABASE db_will_get_dropped;
+CREATE TABLESPACE ts1 LOCATION '$tsprimary';
+CREATE TABLE will_not_change_in_ts (a int, b text) TABLESPACE ts1;
+INSERT INTO will_not_change_in_ts VALUES (1, 'initial test row');
+CREATE TABLE will_change_in_ts (a int, b text) TABLESPACE ts1;
+INSERT INTO will_change_in_ts VALUES (1, 'initial test row');
+CREATE TABLE will_get_dropped_in_ts (a int, b text);
+INSERT INTO will_get_dropped_in_ts VALUES (1, 'initial test row');
EOM
+# Read list of tablespace OIDs. There should be just one.
+my @tsoids = grep { /^\d+/ } slurp_dir($primary->data_dir . '/pg_tblspc');
+is(0+@tsoids, 1, "exactly one user-defined tablespace");
+my $tsoid = $tsoids[0];
+
# Take a full backup.
my $backup1path = $primary->backup_dir . '/backup1';
+my $tsbackup1path = $tempdir . '/ts1backup';
+mkdir($tsbackup1path) || die "mkdir $tsbackup1path: $!";
$primary->command_ok(
- [ 'pg_basebackup', '-D', $backup1path, '--no-sync', '-cfast' ],
- "full backup");
+ [ 'pg_basebackup', '-D', $backup1path, '--no-sync', '-cfast',
+ "-T${tsprimary}=${tsbackup1path}" ], "full backup");
# Now make some database changes.
$primary->safe_psql('postgres', <<EOM);
UPDATE will_change SET b = 'modified value' WHERE a = 1;
+UPDATE will_change_in_ts SET b = 'modified value' WHERE a = 1;
INSERT INTO will_grow
SELECT g, 'additional row' FROM generate_series(2, 5000) g;
TRUNCATE will_shrink;
VACUUM will_get_vacuumed;
DROP TABLE will_get_dropped;
+DROP TABLE will_get_dropped_in_ts;
CREATE TABLE newly_created (a int, b text);
INSERT INTO newly_created VALUES (1, 'row for new table');
+CREATE TABLE newly_created_in_ts (a int, b text) TABLESPACE ts1;
+INSERT INTO newly_created_in_ts VALUES (1, 'row for new table');
VACUUM FULL will_get_rewritten;
DROP DATABASE db_will_get_dropped;
CREATE DATABASE db_newly_created;
@@ -54,8 +76,11 @@ EOM
# Take an incremental backup.
my $backup2path = $primary->backup_dir . '/backup2';
+my $tsbackup2path = $tempdir . '/tsbackup2';
+mkdir($tsbackup2path) || die "mkdir $tsbackup2path: $!";
$primary->command_ok(
[ 'pg_basebackup', '-D', $backup2path, '--no-sync', '-cfast',
+ "-T${tsprimary}=${tsbackup2path}",
'--incremental', $backup1path . '/backup_manifest' ],
"incremental backup");
@@ -78,9 +103,11 @@ $primary->poll_query_until('postgres', $archive_wait_query)
# Perform PITR from the full backup. Disable archive_mode so that the archive
# doesn't find out about the new timeline; that way, the later PITR below will
# choose the same timeline.
+my $tspitr1path = $tempdir . '/tspitr1';
my $pitr1 = PostgreSQL::Test::Cluster->new('pitr1');
$pitr1->init_from_backup($primary, 'backup1',
- standby => 1, has_restoring => 1);
+ standby => 1, has_restoring => 1,
+ tablespace_map => { $tsoid => $tspitr1path });
$pitr1->append_conf('postgresql.conf', qq{
recovery_target_lsn = '$lsn'
recovery_target_action = 'promote'
@@ -90,10 +117,12 @@ $pitr1->start();
# Perform PITR to the same LSN from the incremental backup. Use the same
# basic configuration as before.
+my $tspitr2path = $tempdir . '/tspitr2';
my $pitr2 = PostgreSQL::Test::Cluster->new('pitr2');
$pitr2->init_from_backup($primary, 'backup2',
standby => 1, has_restoring => 1,
- combine_with_prior => [ 'backup1' ]);
+ combine_with_prior => [ 'backup1' ],
+ tablespace_map => { $tsbackup2path => $tspitr2path });
$pitr2->append_conf('postgresql.conf', qq{
recovery_target_lsn = '$lsn'
recovery_target_action = 'promote'
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 9b2879c145..aa9646329f 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -777,7 +777,7 @@ sub backup_fs_cold
=pod
-=item $node->init_from_backup(root_node, backup_name)
+=item $node->init_from_backup(root_node, backup_name, %params)
Initialize a node from a backup, which may come from this node or a different
node. root_node must be a PostgreSQL::Test::Cluster reference, backup_name the string name
@@ -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 combine_with_prior is used,
+the hash keys are the tablespace pathnames used in the backup; otherwise,
+they are tablespace OIDs. In either case, the values are the tablespace
+pathnames that should be used for the target cluster.
To restore from an incremental backup, pass the parameter combine_with_prior
as a reference to an array of prior backup names with which this backup
@@ -843,12 +848,20 @@ sub init_from_backup
}
local %ENV = $self->_get_env();
- PostgreSQL::Test::Utils::system_or_bail('pg_combinebackup', '-d',
- @prior_backup_path, $backup_path, '-o', $data_path);
+ my @combineargs = ('pg_combinebackup', '-d');
+ if (exists $params{tablespace_map})
+ {
+ while (my ($olddir, $newdir) = each %{$params{tablespace_map}})
+ {
+ push @combineargs, "-T$olddir=$newdir";
+ }
+ }
+ push @combineargs, @prior_backup_path, $backup_path, '-o', $data_path;
+ PostgreSQL::Test::Utils::system_or_bail(@combineargs);
}
elsif (defined $params{tar_program})
{
- mkdir($data_path);
+ mkdir($data_path) || die "mkdir $data_path: $!";
PostgreSQL::Test::Utils::system_or_bail($params{tar_program}, 'xf',
$backup_path . '/base.tar',
'-C', $data_path);
@@ -856,11 +869,80 @@ sub init_from_backup
$params{tar_program}, 'xf',
$backup_path . '/pg_wal.tar', '-C',
$data_path . '/pg_wal');
+
+ # We need to generate a tablespace_map file.
+ open(my $tsmap, ">", "$data_path/tablespace_map")
+ || die "$data_path/tablespace_map: $!";
+
+ # Extract tarfiles and add tablespace_map entries
+ my @tstars = grep { /^\d+.tar/ }
+ PostgreSQL::Test::Utils::slurp_dir($backup_path);
+ for my $tstar (@tstars)
+ {
+ my $tsoid = $tstar;
+ $tsoid =~ s/\.tar$//;
+
+ die "no tablespace mapping for $tstar"
+ if !exists $params{tablespace_map} ||
+ !exists $params{tablespace_map}{$tsoid};
+ my $newdir = $params{tablespace_map}{$tsoid};
+
+ mkdir($newdir) || die "mkdir $newdir: $!";
+ PostgreSQL::Test::Utils::system_or_bail($params{tar_program}, 'xf',
+ $backup_path . '/' . $tstar, '-C', $newdir);
+
+ my $escaped_newdir = $newdir;
+ $escaped_newdir =~ s/\\/\\\\/g;
+ print $tsmap "$tsoid $escaped_newdir\n";
+ }
+
+ # Close tablespace_map.
+ close($tsmap);
}
else
{
+ my @tsoids;
rmdir($data_path);
- PostgreSQL::Test::RecursiveCopy::copypath($backup_path, $data_path);
+
+ # Copy the main backup. If we see a tablespace directory for which we
+ # have a tablespace mapping, skip it, but remember that we saw it.
+ PostgreSQL::Test::RecursiveCopy::copypath($backup_path, $data_path,
+ 'filterfn' => sub {
+ my ($path) = @_;
+ if ($path =~ /^pg_tblspc\/(\d+)$/ &&
+ exists $params{tablespace_map}{$1})
+ {
+ push @tsoids, $1;
+ return 0;
+ }
+ return 1;
+ });
+
+ if (@tsoids > 0)
+ {
+ # We need to generate a tablespace_map file.
+ open(my $tsmap, ">", "$data_path/tablespace_map")
+ || die "$data_path/tablespace_map: $!";
+
+ # Now use the list of tablespace links to copy each tablespace.
+ for my $tsoid (@tsoids)
+ {
+ die "no tablespace mapping for $tsoid"
+ if !exists $params{tablespace_map} ||
+ !exists $params{tablespace_map}{$tsoid};
+
+ my $olddir = $backup_path . '/pg_tblspc/' . $tsoid;
+ my $newdir = $params{tablespace_map}{$tsoid};
+ PostgreSQL::Test::RecursiveCopy::copypath($olddir, $newdir);
+
+ my $escaped_newdir = $newdir;
+ $escaped_newdir =~ s/\\/\\\\/g;
+ print $tsmap "$tsoid $escaped_newdir\n";
+ }
+
+ # Close tablespace_map.
+ close($tsmap);
+ }
}
chmod(0700, $data_path) or die $!;
--
2.39.3 (Apple Git-145)
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