pg_combinebackup --clone doesn't work
The pg_combinebackup --clone option currently doesn't work at all. Even
on systems where it should it be supported, you'll always get a "file
cloning not supported on this platform" error.
The reason is this checking code in pg_combinebackup.c:
#if (defined(HAVE_COPYFILE) && defined(COPYFILE_CLONE_FORCE)) || \
(defined(__linux__) && defined(FICLONE))
if (opt.dry_run)
pg_log_debug("would use cloning to copy files");
else
pg_log_debug("will use cloning to copy files");
#else
pg_fatal("file cloning not supported on this platform");
#endif
The problem is that this file does not include the appropriate OS header
files that would define COPYFILE_CLONE_FORCE or FICLONE, respectively.
The same problem also exists in copy_file.c. (That one has the right
header file for macOS but still not for Linux.)
This should be pretty easy to fix up, and we should think about some
ways to refactor this to avoid repeating all these OS-specific things a
few times. (The code was copied from pg_upgrade originally.)
But in the short term, how about some test coverage? You can exercise
the different pg_combinebackup copy modes like this:
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm
b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 83f385a4870..7e8dd024c82 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -848,7 +848,7 @@ sub init_from_backup
} local %ENV = $self->_get_env();
- my @combineargs = ('pg_combinebackup', '-d');
+ my @combineargs = ('pg_combinebackup', '-d', '--clone');
if (exists $params{tablespace_map})
{
while (my ($olddir, $newdir) = each %{
$params{tablespace_map} })We could do something like what we have for pg_upgrade, where we can use
the environment variable PG_TEST_PG_UPGRADE_MODE to test the different
copy modes. We could turn this into a global option. (This might also
be useful for future work to use file cloning elsewhere, like in CREATE
DATABASE?)
Also, I think it would be useful for consistency if pg_combinebackup had
a --copy option to select the default mode, like pg_upgrade does.
On 6/20/24 07:55, Peter Eisentraut wrote:
The pg_combinebackup --clone option currently doesn't work at all. Even
on systems where it should it be supported, you'll always get a "file
cloning not supported on this platform" error.The reason is this checking code in pg_combinebackup.c:
#if (defined(HAVE_COPYFILE) && defined(COPYFILE_CLONE_FORCE)) || \
(defined(__linux__) && defined(FICLONE))if (opt.dry_run)
pg_log_debug("would use cloning to copy files");
else
pg_log_debug("will use cloning to copy files");#else
pg_fatal("file cloning not supported on this platform");
#endifThe problem is that this file does not include the appropriate OS header
files that would define COPYFILE_CLONE_FORCE or FICLONE, respectively.The same problem also exists in copy_file.c. (That one has the right
header file for macOS but still not for Linux.)
Seems like my bug, I guess :-( Chances are the original patches had the
include, but it got lost during refactoring or something. Anyway, will
fix shortly.
This should be pretty easy to fix up, and we should think about some
ways to refactor this to avoid repeating all these OS-specific things a
few times. (The code was copied from pg_upgrade originally.)
Yeah. The ifdef forest got rather hard to navigate.
But in the short term, how about some test coverage? You can exercise
the different pg_combinebackup copy modes like this:diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm index 83f385a4870..7e8dd024c82 100644 --- a/src/test/perl/PostgreSQL/Test/Cluster.pm +++ b/src/test/perl/PostgreSQL/Test/Cluster.pm @@ -848,7 +848,7 @@ sub init_from_backup }local %ENV = $self->_get_env(); - my @combineargs = ('pg_combinebackup', '-d'); + my @combineargs = ('pg_combinebackup', '-d', '--clone'); if (exists $params{tablespace_map}) { while (my ($olddir, $newdir) = each %{ $params{tablespace_map} })
For ad hoc testing? Sure, but that won't work on platforms without the
clone support, right?
We could do something like what we have for pg_upgrade, where we can use
the environment variable PG_TEST_PG_UPGRADE_MODE to test the different
copy modes. We could turn this into a global option. (This might also
be useful for future work to use file cloning elsewhere, like in CREATE
DATABASE?)
Yeah, this sounds like a good way to do this. Is there a good reason to
have multiple different variables, one for each tool, or should we have
a single PG_TEST_COPY_MODE affecting all the places?
Also, I think it would be useful for consistency if pg_combinebackup had
a --copy option to select the default mode, like pg_upgrade does.
I vaguely recall this might have been discussed in the thread about
adding cloning to pg_combinebackup, but I don't recall the details why
we didn't adopt the pg_uprade way. But we can revisit that, IMO.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
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.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
0001-fix-clone-headers.patchtext/x-patch; charset=UTF-8; name=0001-fix-clone-headers.patchDownload
From ae712aa6316c8f7035edee9fb49e1bfe1ea30b94 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
Date: Thu, 20 Jun 2024 23:50:22 +0200
Subject: [PATCH] fix clone headers
---
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
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
Attachments:
0001-Add-headers-needed-by-pg_combinebackup-clone.patchtext/x-patch; charset=UTF-8; name=0001-Add-headers-needed-by-pg_combinebackup-clone.patchDownload
From 94082624db5d7608f3fb7a5b9322ee90dc5cfcb5 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@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
0002-Introduce-pg_combinebackup-copy-option.patchtext/x-patch; charset=UTF-8; name=0002-Introduce-pg_combinebackup-copy-option.patchDownload
From 93ecbe611936279127cebc5ddfefb6ebf1f4730f Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@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
0003-Add-PG_TEST_PG_COMBINEBACKUP_MODE.patchtext/x-patch; charset=UTF-8; name=0003-Add-PG_TEST_PG_COMBINEBACKUP_MODE.patchDownload
From 0b3a9afe25d8837c0dcf5a03b856c8db1f4c26cd Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@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
0004-Add-PG_TEST_PG_COMBINEBACKUP_MODE-to-CI-tasks.patchtext/x-patch; charset=UTF-8; name=0004-Add-PG_TEST_PG_COMBINEBACKUP_MODE-to-CI-tasks.patchDownload
From 836e20a0ed34b7a9c367674e771fa8cb023e4341 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@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
On 21.06.24 18:10, Tomas Vondra wrote:
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
This looks good.
0003 adds the PG_TEST_PG_COMBINEBACKUP_MODE, so that we can override the
copy method for tests
I had imagined that we combine PG_TEST_PG_UPGRADE_MODE and this new one
into one setting. But maybe that's something to consider with less time
pressure for PG18.
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.
Right, that was one of the reasons.
0004 tweaks two of the Cirrus CI tasks to use --clone/--copy-file-range
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)?
It's tricky to find the right balance here. We had to figure this out
for pg_upgrade as well. I think your solution is good, and we should
also add test coverage for pg_upgrade --copy-file-range in the same
place, I think.
On 6/25/24 15:21, Peter Eisentraut wrote:
On 21.06.24 18:10, Tomas Vondra wrote:
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
This looks good.
0003 adds the PG_TEST_PG_COMBINEBACKUP_MODE, so that we can override the
copy method for testsI had imagined that we combine PG_TEST_PG_UPGRADE_MODE and this new one
into one setting. But maybe that's something to consider with less time
pressure for PG18.
Yeah. I initially planned to combine those options into a single one,
because it seems like it's not very useful to have them set differently,
and because it's easier to not have different options between releases.
But then I realized PG_TEST_PG_UPGRADE_MODE was added in 16, so this
ship already sailed - so no reason to rush this into 18.
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.Right, that was one of the reasons.
0004 tweaks two of the Cirrus CI tasks to use --clone/--copy-file-range
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)?It's tricky to find the right balance here. We had to figure this out
for pg_upgrade as well. I think your solution is good, and we should
also add test coverage for pg_upgrade --copy-file-range in the same
place, I think.
Yeah. I'm not sure if we need to decide this now, or if we can tweak the
testing even for released branches.
IMHO the main challenge is to decide which combinations we actually want
to test on CI. It'd be nice to test each platform with all modes it
supports (I guess for backups that wouldn't be a bad thing). But that'd
require expanding the number of combinations, and I don't think that's
likely.
Maybe it'd be possible to have a second CI config, with additional
combinations, but not triggered after each push?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hi,
I've pushed the first three patches, fixing the headers, adding the
--copy option and PG_TEST_PG_COMBINEBACKUP_MODE variable.
I haven't pushed the CI changes, I'm not sure if there's a clear
consensus on which combination to test. It's something we can tweak
later, I think.
FWIW I've added the patch to the 2024-07 commitfest, but mostly to get
some CI runs (runs on private fork fail with some macos issues unrelated
to the patch).
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
0001-Add-PG_TEST_PG_COMBINEBACKUP_MODE-to-CI-tasks.patchtext/x-patch; charset=UTF-8; name=0001-Add-PG_TEST_PG_COMBINEBACKUP_MODE-to-CI-tasks.patchDownload
From c92cc862026d1d2e48dd97e05f497873fb852588 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
Date: Sun, 30 Jun 2024 19:03:02 +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
On 30.06.24 20:58, Tomas Vondra wrote:
I've pushed the first three patches, fixing the headers, adding the
--copy option and PG_TEST_PG_COMBINEBACKUP_MODE variable.I haven't pushed the CI changes, I'm not sure if there's a clear
consensus on which combination to test. It's something we can tweak
later, I think.FWIW I've added the patch to the 2024-07 commitfest, but mostly to get
some CI runs (runs on private fork fail with some macos issues unrelated
to the patch).
This last patch is still pending in the commitfest. Personally, I think
it's good to commit as is.
On 8/23/24 14:00, Peter Eisentraut wrote:
On 30.06.24 20:58, Tomas Vondra wrote:
I've pushed the first three patches, fixing the headers, adding the
--copy option and PG_TEST_PG_COMBINEBACKUP_MODE variable.I haven't pushed the CI changes, I'm not sure if there's a clear
consensus on which combination to test. It's something we can tweak
later, I think.FWIW I've added the patch to the 2024-07 commitfest, but mostly to get
some CI runs (runs on private fork fail with some macos issues unrelated
to the patch).This last patch is still pending in the commitfest. Personally, I think
it's good to commit as is.
OK, thanks for reminding me. I'll take care of it after thinking about
it a bit more.
regards
--
Tomas Vondra
On 8/23/24 14:50, Tomas Vondra wrote:
On 8/23/24 14:00, Peter Eisentraut wrote:
On 30.06.24 20:58, Tomas Vondra wrote:
I've pushed the first three patches, fixing the headers, adding the
--copy option and PG_TEST_PG_COMBINEBACKUP_MODE variable.I haven't pushed the CI changes, I'm not sure if there's a clear
consensus on which combination to test. It's something we can tweak
later, I think.FWIW I've added the patch to the 2024-07 commitfest, but mostly to get
some CI runs (runs on private fork fail with some macos issues unrelated
to the patch).This last patch is still pending in the commitfest. Personally, I think
it's good to commit as is.OK, thanks for reminding me. I'll take care of it after thinking about
it a bit more.
Took me longer than I expected, but pushed (into master only).
regards
--
Tomas Vondra