On 6/21/24 00:07, Tomas Vondra wrote:
> Here's a fix adding the missing headers to pg_combinebackup, and fixing
> some compile-time issues in the ifdef-ed block.
>
> I've done some basic manual testing today - I plan to test this a bit
> more tomorrow, and I'll also look at integrating this into the existing
> tests.
>
Here's a bit more complete / cleaned patch, adding the testing changes
in separate parts.
0001 adds the missing headers / fixes the now-accessible code a bit
0002 adds the --copy option for consistency with pg_upgrade
0003 adds the PG_TEST_PG_COMBINEBACKUP_MODE, so that we can override the
copy method for tests
0004 tweaks two of the Cirrus CI tasks to use --clone/--copy-file-range
I believe 0001-0003 are likely non-controversial, although if someone
could take a look at the Perl in 0003 that'd be nice. Also, 0002 seems
nice not only because of consistency with pg_upgrade, but it also makes
0003 easier as we don't need to special-case the default mode etc.
I'm not sure about 0004 - I initially did this mostly to check we have
the right headers on other platforms, but not sure we want to actually
do this. Or maybe we want to test a different combination (e.g. also
test the --clone on Linux)?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From 94082624db5d7608f3fb7a5b9322ee90dc5cfcb5 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@2ndquadrant.com>
Date: Thu, 20 Jun 2024 23:50:22 +0200
Subject: [PATCH 1/4] Add headers needed by pg_combinebackup --clone
The code for cloning files existed, but was inaccessible as it relied on
constants from missing headers. The consequence is that on Linux --clone
always failed with
error: file cloning not supported on this platform
Fixed by including the missing headers to relevant places. This revealed
a couple compile errors in copy_file_clone(), so fix those too.
Reported-by: Peter Eisentraut
Discussion: https://postgr.es/m/48da4a1f-ccd9-4988-9622-24f37b1de2b4%40eisentraut.org
---
src/bin/pg_combinebackup/copy_file.c | 12 +++++++++++-
src/bin/pg_combinebackup/pg_combinebackup.c | 8 ++++++++
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_combinebackup/copy_file.c b/src/bin/pg_combinebackup/copy_file.c
index 08c3b875420..8b50e937346 100644
--- a/src/bin/pg_combinebackup/copy_file.c
+++ b/src/bin/pg_combinebackup/copy_file.c
@@ -13,6 +13,10 @@
#ifdef HAVE_COPYFILE_H
#include <copyfile.h>
#endif
+#ifdef __linux__
+#include <sys/ioctl.h>
+#include <linux/fs.h>
+#endif
#include <fcntl.h>
#include <limits.h>
#include <sys/stat.h>
@@ -214,6 +218,9 @@ copy_file_clone(const char *src, const char *dest,
pg_fatal("error while cloning file \"%s\" to \"%s\": %m", src, dest);
#elif defined(__linux__) && defined(FICLONE)
{
+ int src_fd;
+ int dest_fd;
+
if ((src_fd = open(src, O_RDONLY | PG_BINARY, 0)) < 0)
pg_fatal("could not open file \"%s\": %m", src);
@@ -228,8 +235,11 @@ copy_file_clone(const char *src, const char *dest,
unlink(dest);
pg_fatal("error while cloning file \"%s\" to \"%s\": %s",
- src, dest);
+ src, dest, strerror(save_errno));
}
+
+ close(src_fd);
+ close(dest_fd);
}
#else
pg_fatal("file cloning not supported on this platform");
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index 01d7fb98e79..f67ccf76ea2 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -16,6 +16,14 @@
#include <fcntl.h>
#include <limits.h>
+#ifdef HAVE_COPYFILE_H
+#include <copyfile.h>
+#endif
+#ifdef __linux__
+#include <sys/ioctl.h>
+#include <linux/fs.h>
+#endif
+
#include "backup_label.h"
#include "common/blkreftable.h"
#include "common/checksum_helper.h"
--
2.45.2
From 93ecbe611936279127cebc5ddfefb6ebf1f4730f Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@2ndquadrant.com>
Date: Fri, 21 Jun 2024 14:36:04 +0200
Subject: [PATCH 2/4] Introduce pg_combinebackup --copy option
The --copy option simply allows picking the default mode to copy data,
and does not change the behavior. This makes pg_combinebackup options
more consistent with pg_upgrade.
Reported-by: Peter Eisentraut
Discussion: https://postgr.es/m/48da4a1f-ccd9-4988-9622-24f37b1de2b4%40eisentraut.org
---
doc/src/sgml/ref/pg_combinebackup.sgml | 10 ++++++++++
src/bin/pg_combinebackup/pg_combinebackup.c | 7 ++++++-
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/pg_combinebackup.sgml b/doc/src/sgml/ref/pg_combinebackup.sgml
index 375307d57bd..091982f62ad 100644
--- a/doc/src/sgml/ref/pg_combinebackup.sgml
+++ b/doc/src/sgml/ref/pg_combinebackup.sgml
@@ -162,6 +162,16 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--copy</option></term>
+ <listitem>
+ <para>
+ Perform regular file copy. This is the default. (See also
+ <option>--copy-file-range</option> and <option>--clone</option>.)
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>--copy-file-range</option></term>
<listitem>
diff --git a/src/bin/pg_combinebackup/pg_combinebackup.c b/src/bin/pg_combinebackup/pg_combinebackup.c
index f67ccf76ea2..6ddf5b534b1 100644
--- a/src/bin/pg_combinebackup/pg_combinebackup.c
+++ b/src/bin/pg_combinebackup/pg_combinebackup.c
@@ -139,7 +139,8 @@ main(int argc, char *argv[])
{"no-manifest", no_argument, NULL, 2},
{"sync-method", required_argument, NULL, 3},
{"clone", no_argument, NULL, 4},
- {"copy-file-range", no_argument, NULL, 5},
+ {"copy", no_argument, NULL, 5},
+ {"copy-file-range", no_argument, NULL, 6},
{NULL, 0, NULL, 0}
};
@@ -209,6 +210,9 @@ main(int argc, char *argv[])
opt.copy_method = COPY_METHOD_CLONE;
break;
case 5:
+ opt.copy_method = COPY_METHOD_COPY;
+ break;
+ case 6:
opt.copy_method = COPY_METHOD_COPY_FILE_RANGE;
break;
default:
@@ -763,6 +767,7 @@ help(const char *progname)
printf(_(" -T, --tablespace-mapping=OLDDIR=NEWDIR\n"
" relocate tablespace in OLDDIR to NEWDIR\n"));
printf(_(" --clone clone (reflink) instead of copying files\n"));
+ printf(_(" --copy copy files (default)\n"));
printf(_(" --copy-file-range copy using copy_file_range() syscall\n"));
printf(_(" --manifest-checksums=SHA{224,256,384,512}|CRC32C|NONE\n"
" use algorithm for manifest checksums\n"));
--
2.45.2
From 0b3a9afe25d8837c0dcf5a03b856c8db1f4c26cd Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@2ndquadrant.com>
Date: Fri, 21 Jun 2024 14:16:16 +0200
Subject: [PATCH 3/4] Add PG_TEST_PG_COMBINEBACKUP_MODE
This introduces an environment variable PG_TEST_PG_COMBINEBACKUP_MODE
that determines copy mode used by pg_combinebackup in TAP tests. This
defaults to "--copy" but may be set to "--clone" / "--copy-file-range"
to use the alternative stategies.
Reported-by: Peter Eisentraut
Discussion: https://postgr.es/m/48da4a1f-ccd9-4988-9622-24f37b1de2b4%40eisentraut.org
---
.../pg_combinebackup/t/002_compare_backups.pl | 8 +++++-
src/bin/pg_combinebackup/t/003_timeline.pl | 8 +++++-
src/bin/pg_combinebackup/t/004_manifest.pl | 9 +++++--
src/bin/pg_combinebackup/t/005_integrity.pl | 25 +++++++++++--------
.../pg_combinebackup/t/006_db_file_copy.pl | 8 +++++-
src/test/perl/PostgreSQL/Test/Cluster.pm | 5 ++++
6 files changed, 48 insertions(+), 15 deletions(-)
diff --git a/src/bin/pg_combinebackup/t/002_compare_backups.pl b/src/bin/pg_combinebackup/t/002_compare_backups.pl
index f032959ef5c..63a0255de15 100644
--- a/src/bin/pg_combinebackup/t/002_compare_backups.pl
+++ b/src/bin/pg_combinebackup/t/002_compare_backups.pl
@@ -9,6 +9,11 @@ use Test::More;
my $tempdir = PostgreSQL::Test::Utils::tempdir_short();
+# Can be changed to test the other modes.
+my $mode = $ENV{PG_TEST_PG_COMBINEBACKUP_MODE} || '--copy';
+
+note "testing using mode $mode";
+
# Set up a new database instance.
my $primary = PostgreSQL::Test::Cluster->new('primary');
$primary->init(has_archiving => 1, allows_streaming => 1);
@@ -134,7 +139,8 @@ $pitr2->init_from_backup(
standby => 1,
has_restoring => 1,
combine_with_prior => ['backup1'],
- tablespace_map => { $tsbackup2path => $tspitr2path });
+ tablespace_map => { $tsbackup2path => $tspitr2path },
+ combine_mode => $mode);
$pitr2->append_conf(
'postgresql.conf', qq{
recovery_target_lsn = '$lsn'
diff --git a/src/bin/pg_combinebackup/t/003_timeline.pl b/src/bin/pg_combinebackup/t/003_timeline.pl
index 52eb642a392..83ab674a244 100644
--- a/src/bin/pg_combinebackup/t/003_timeline.pl
+++ b/src/bin/pg_combinebackup/t/003_timeline.pl
@@ -10,6 +10,11 @@ use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
+# Can be changed to test the other modes.
+my $mode = $ENV{PG_TEST_PG_COMBINEBACKUP_MODE} || '--copy';
+
+note "testing using mode $mode";
+
# Set up a new database instance.
my $node1 = PostgreSQL::Test::Cluster->new('node1');
$node1->init(has_archiving => 1, allows_streaming => 1);
@@ -68,7 +73,8 @@ $node2->command_ok(
# Restore the incremental backup and use it to create a new node.
my $node3 = PostgreSQL::Test::Cluster->new('node3');
$node3->init_from_backup($node1, 'backup3',
- combine_with_prior => [ 'backup1', 'backup2' ]);
+ combine_with_prior => [ 'backup1', 'backup2' ],
+ combine_mode => $mode);
$node3->start();
# Let's insert one more row.
diff --git a/src/bin/pg_combinebackup/t/004_manifest.pl b/src/bin/pg_combinebackup/t/004_manifest.pl
index 0df9fed73a8..6d475163ab9 100644
--- a/src/bin/pg_combinebackup/t/004_manifest.pl
+++ b/src/bin/pg_combinebackup/t/004_manifest.pl
@@ -12,6 +12,11 @@ use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
+# Can be changed to test the other modes.
+my $mode = $ENV{PG_TEST_PG_COMBINEBACKUP_MODE} || '--copy';
+
+note "testing using mode $mode";
+
# Set up a new database instance.
my $node = PostgreSQL::Test::Cluster->new('node');
$node->init(has_archiving => 1, allows_streaming => 1);
@@ -53,9 +58,9 @@ sub combine_and_test_one_backup
combine_and_test_one_backup('nomanifest',
qr/could not open file.*backup_manifest/,
'--no-manifest');
-combine_and_test_one_backup('csum_none', undef, '--manifest-checksums=NONE');
+combine_and_test_one_backup('csum_none', undef, '--manifest-checksums=NONE', $mode);
combine_and_test_one_backup('csum_sha224',
- undef, '--manifest-checksums=SHA224');
+ undef, '--manifest-checksums=SHA224', $mode);
# Verify that SHA224 is mentioned in the SHA224 manifest lots of times.
my $sha224_manifest =
diff --git a/src/bin/pg_combinebackup/t/005_integrity.pl b/src/bin/pg_combinebackup/t/005_integrity.pl
index 636c3cc1b14..3caed13f6ed 100644
--- a/src/bin/pg_combinebackup/t/005_integrity.pl
+++ b/src/bin/pg_combinebackup/t/005_integrity.pl
@@ -13,6 +13,11 @@ use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
+# Can be changed to test the other modes.
+my $mode = $ENV{PG_TEST_PG_COMBINEBACKUP_MODE} || '--copy';
+
+note "testing using mode $mode";
+
# Set up a new database instance.
my $node1 = PostgreSQL::Test::Cluster->new('node1');
$node1->init(has_archiving => 1, allows_streaming => 1);
@@ -79,13 +84,13 @@ my $resultpath = $node1->backup_dir . '/result';
# Can't combine 2 full backups.
$node1->command_fails_like(
- [ 'pg_combinebackup', $backup1path, $backup1path, '-o', $resultpath ],
+ [ 'pg_combinebackup', $backup1path, $backup1path, '-o', $resultpath, $mode ],
qr/is a full backup, but only the first backup should be a full backup/,
"can't combine full backups");
# Can't combine 2 incremental backups.
$node1->command_fails_like(
- [ 'pg_combinebackup', $backup2path, $backup2path, '-o', $resultpath ],
+ [ 'pg_combinebackup', $backup2path, $backup2path, '-o', $resultpath, $mode ],
qr/is an incremental backup, but the first backup should be a full backup/,
"can't combine full backups");
@@ -93,7 +98,7 @@ $node1->command_fails_like(
$node1->command_fails_like(
[
'pg_combinebackup', $backup1path, $backupother2path, '-o',
- $resultpath
+ $resultpath, $mode
],
qr/expected system identifier.*but found/,
"can't combine backups from different nodes");
@@ -106,7 +111,7 @@ copy("$backupother2path/backup_manifest", "$backup2path/backup_manifest")
$node1->command_fails_like(
[
'pg_combinebackup', $backup1path, $backup2path, $backup3path,
- '-o', $resultpath
+ '-o', $resultpath, $mode
],
qr/ manifest system identifier is .*, but control file has /,
"can't combine backups with different manifest system identifier ");
@@ -116,7 +121,7 @@ move("$backup2path/backup_manifest.orig", "$backup2path/backup_manifest")
# Can't omit a required backup.
$node1->command_fails_like(
- [ 'pg_combinebackup', $backup1path, $backup3path, '-o', $resultpath ],
+ [ 'pg_combinebackup', $backup1path, $backup3path, '-o', $resultpath, $mode ],
qr/starts at LSN.*but expected/,
"can't omit a required backup");
@@ -124,7 +129,7 @@ $node1->command_fails_like(
$node1->command_fails_like(
[
'pg_combinebackup', $backup1path, $backup3path, $backup2path,
- '-o', $resultpath
+ '-o', $resultpath, $mode
],
qr/starts at LSN.*but expected/,
"can't combine backups in the wrong order");
@@ -133,7 +138,7 @@ $node1->command_fails_like(
$node1->command_ok(
[
'pg_combinebackup', $backup1path, $backup2path, $backup3path,
- '-o', $resultpath
+ '-o', $resultpath, $mode
],
"can combine 3 matching backups");
rmtree($resultpath);
@@ -143,19 +148,19 @@ my $synthetic12path = $node1->backup_dir . '/synthetic12';
$node1->command_ok(
[
'pg_combinebackup', $backup1path, $backup2path, '-o',
- $synthetic12path
+ $synthetic12path, $mode
],
"can combine 2 matching backups");
# Can combine result of previous step with second incremental.
$node1->command_ok(
- [ 'pg_combinebackup', $synthetic12path, $backup3path, '-o', $resultpath ],
+ [ 'pg_combinebackup', $synthetic12path, $backup3path, '-o', $resultpath, $mode ],
"can combine synthetic backup with later incremental");
rmtree($resultpath);
# Can't combine result of 1+2 with 2.
$node1->command_fails_like(
- [ 'pg_combinebackup', $synthetic12path, $backup2path, '-o', $resultpath ],
+ [ 'pg_combinebackup', $synthetic12path, $backup2path, '-o', $resultpath, $mode ],
qr/starts at LSN.*but expected/,
"can't combine synthetic backup with included incremental");
diff --git a/src/bin/pg_combinebackup/t/006_db_file_copy.pl b/src/bin/pg_combinebackup/t/006_db_file_copy.pl
index d57b550af21..f44788e82bf 100644
--- a/src/bin/pg_combinebackup/t/006_db_file_copy.pl
+++ b/src/bin/pg_combinebackup/t/006_db_file_copy.pl
@@ -7,6 +7,11 @@ use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;
+# Can be changed to test the other modes.
+my $mode = $ENV{PG_TEST_PG_COMBINEBACKUP_MODE} || '--copy';
+
+note "testing using mode $mode";
+
# Set up a new database instance.
my $primary = PostgreSQL::Test::Cluster->new('primary');
$primary->init(has_archiving => 1, allows_streaming => 1);
@@ -45,7 +50,8 @@ $primary->command_ok(
# Recover the incremental backup.
my $restore = PostgreSQL::Test::Cluster->new('restore');
$restore->init_from_backup($primary, 'backup2',
- combine_with_prior => ['backup1']);
+ combine_with_prior => ['backup1'],
+ combine_mode => $mode);
$restore->start();
# Query the DB.
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 83f385a4870..0135c5a795c 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -856,6 +856,11 @@ sub init_from_backup
push @combineargs, "-T$olddir=$newdir";
}
}
+ # use the combine mode (clone/copy-file-range) if specified
+ if (defined $params{combine_mode})
+ {
+ push @combineargs, $params{combine_mode};
+ }
push @combineargs, @prior_backup_path, $backup_path, '-o', $data_path;
PostgreSQL::Test::Utils::system_or_bail(@combineargs);
}
--
2.45.2
From 836e20a0ed34b7a9c367674e771fa8cb023e4341 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <to...@2ndquadrant.com>
Date: Fri, 21 Jun 2024 17:55:38 +0200
Subject: [PATCH 4/4] Add PG_TEST_PG_COMBINEBACKUP_MODE to CI tasks
This changes pg_combinebackup mode for two of the Cirrus CI tasks, to
exercise the alternative copy methods in pg_combinebackup tests. We
test --clone on macOS and --copy-file-range on one of the Linux tasks.
Reported-by: Peter Eisentraut
Discussion: https://postgr.es/m/48da4a1f-ccd9-4988-9622-24f37b1de2b4%40eisentraut.org
---
.cirrus.tasks.yml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index 33646faeadf..4e029103c7e 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -318,6 +318,7 @@ task:
env:
SANITIZER_FLAGS: -fsanitize=address
+ PG_TEST_PG_COMBINEBACKUP_MODE: --copy-file-range
# Normally, the "relation segment" code basically has no coverage in our
# tests, because we (quite reasonably) don't generate tables large
@@ -432,6 +433,7 @@ task:
CXXFLAGS: -Og -ggdb
PG_TEST_PG_UPGRADE_MODE: --clone
+ PG_TEST_PG_COMBINEBACKUP_MODE: --clone
<<: *macos_task_template
--
2.45.2
