Thread: 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");
> #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.)
>
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
Attachment
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
Attachment
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 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. > 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
Attachment
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