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
