Subject: Re: pg_combinebackup --clone doesn't work

From: Peter Eisentraut <[email-masked]>
Date: Tue, 25 Jun 2024 15:21:37 +0200

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.

---

From: Tomas Vondra <[email-masked]>
Date: Sun, 30 Jun 2024 20:58:02 +0200

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

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

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 23 Aug 2024 14:00:48 +0200

> some CI runs (runs on private fork fail with some macos issues unrelated

This last patch is still pending in the commitfest.  Personally, I think 
it's good to commit as is.