Subject: pg_combinebackup --clone doesn't work

From: Peter Eisentraut <[email-masked]>
Date: Thu, 20 Jun 2024 07:55:07 +0200

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.

---

From: Tomas Vondra <[email-masked]>
Date: Thu, 20 Jun 2024 11:31:09 +0200

> 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.

> few times.  (The code was copied from pg_upgrade originally.)

Yeah. The ifdef forest got rather hard to navigate.

> $params{tablespace_map} })

For ad hoc testing? Sure, but that won't work on platforms without the
clone support, right?

> be useful for future work to use file cloning elsewhere, like in CREATE

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?

> 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

---

From: Tomas Vondra <[email-masked]>
Date: Fri, 21 Jun 2024 00:07:32 +0200

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

From ae712aa6316c8f7035edee9fb49e1bfe1ea30b94 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email-masked]>
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