Message-ID: <c8b9959e-db27-4dc7-be65-7a32570f0a84@enterprisedb.com>
Date: Thu, 20 Jun 2024 11:31:09 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: pg_combinebackup --clone doesn't work
To: Peter Eisentraut <peter@eisentraut.org>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
References: <48da4a1f-ccd9-4988-9622-24f37b1de2b4@eisentraut.org>
Content-Language: en-US
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <48da4a1f-ccd9-4988-9622-24f37b1de2b4@eisentraut.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/c8b9959e-db27-4dc7-be65-7a32570f0a84%40enterprisedb.com>
Precedence: bulk

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