Re: pg_combinebackup --clone doesn't work

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