Обсуждение: pg_upgrade: Make testing different transfer modes easier

I wanted to test the different pg_upgrade transfer modes (--link, 
--clone), but that was not that easy, because there is more than one 
place in the test script you have to find and manually change.  So I 
wrote a little patch to make that easier.  It's still manual, but it's a 
start.  (In principle, we could automatically run the tests with each 
supported mode in a loop, but that would be very slow.)

While doing that, I also found it strange that the default transfer mode 
(referred to as "copy" internally) did not have any external 
representation, so it is awkward to refer to it in text, and obscure to 
see where it is used for example in those test scripts.  So I added an 
option --copy, which effectively does nothing, but it's not uncommon to 
have options that select default behaviors explicitly.  (I also thought 
about something like a "mode" option with an argument, but given that we 
already have --link and --clone, this seemed the most sensible.)

Thoughts?

At Thu, 1 Dec 2022 16:18:21 +0100, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote in 
> I wanted to test the different pg_upgrade transfer modes (--link,
> --clone), but that was not that easy, because there is more than one
> place in the test script you have to find and manually change.  So I
> wrote a little patch to make that easier.  It's still manual, but it's
> a start.  (In principle, we could automatically run the tests with
> each supported mode in a loop, but that would be very slow.)
> 
> While doing that, I also found it strange that the default transfer
> mode (referred to as "copy" internally) did not have any external
> representation, so it is awkward to refer to it in text, and obscure
> to see where it is used for example in those test scripts.  So I added
> an option --copy, which effectively does nothing, but it's not
> uncommon to have options that select default behaviors explicitly.  (I

I don't have a clear idea of wheter it is common or not, but I suppose many such commands allow to choose the default
behaviorby a configuration file or an environment variable, etc. But I don't mind the command had the effetively nop
optiononly for completeness.
 

> also thought about something like a "mode" option with an argument,
> but given that we already have --link and --clone, this seemed the
> most sensible.)
> 
> Thoughts?

When I read up to the point of the --copy option, what came to my mind
was the --mode=<blah> option.  IMHO, if I was going to add an option
to choose the copy behavior, I would add --mode option instead, like
pg_ctl does, as it implicitly signals that the suboptions are mutually
exclusive.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

> On 1 Dec 2022, at 16:18, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> I wanted to test the different pg_upgrade transfer modes (--link, --clone), but that was not that easy, because there
ismore than one place in the test script you have to find and manually change.  So I wrote a little patch to make that
easier. It's still manual, but it's a start.  (In principle, we could automatically run the tests with each supported
modein a loop, but that would be very slow.) 

Wouldn't it be possible, and less change-code-manual, to accept this via an
extension to PROVE_FLAGS?  Any options after :: to prove are passed to the
test(s) [0] so we could perhaps inspect @ARGV for the mode if we invent a new
way to pass arguments.  Something along the lines of the untested sketch
below in the pg_upgrade test:

+# Optionally set the file transfer mode for the tests via arguments to PROVE
+my $mode = (@ARGV);
+$mode = '--copy' unless defined;

.. together with an extension to Makefile.global.in ..

-   $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
+   $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl) $(PROVE_TEST_ARGS)

.. should *I think* allow for passing the mode to the tests via:

make -C src/bin/pg_upgrade check PROVE_TEST_ARGS=":: --link"

The '::' part should of course ideally be injected automatically but the above
is mostly thinking out loud pseudocode so I didn't add that.

This could probably benefit other tests as well, to make it eas{y|ier} to run
extended testing on certain buildfarm animals or in the CFBot CI on specific
patches in the commitfest.

> While doing that, I also found it strange that the default transfer mode (referred to as "copy" internally) did not
haveany external representation, so it is awkward to refer to it in text, and obscure to see where it is used for
examplein those test scripts.  So I added an option --copy, which effectively does nothing, but it's not uncommon to
haveoptions that select default behaviors explicitly.  (I also thought about something like a "mode" option with an
argument,but given that we already have --link and --clone, this seemed the most sensible.) 

Agreed, +1 on adding --copy regardless of the above.

--
Daniel Gustafsson        https://vmware.com/

[0] https://perldoc.perl.org/prove#Arguments-to-Tests

On 02.12.22 01:56, Kyotaro Horiguchi wrote:
>> also thought about something like a "mode" option with an argument,
>> but given that we already have --link and --clone, this seemed the
>> most sensible.)
>>
>> Thoughts?
> 
> When I read up to the point of the --copy option, what came to my mind
> was the --mode=<blah> option.  IMHO, if I was going to add an option
> to choose the copy behavior, I would add --mode option instead, like
> pg_ctl does, as it implicitly signals that the suboptions are mutually
> exclusive.

Ok, we have sort of one vote for each variant now.  Let's see if there 
are other opinions.

On 02.12.22 13:04, Daniel Gustafsson wrote:
> Wouldn't it be possible, and less change-code-manual, to accept this via an
> extension to PROVE_FLAGS?  Any options after :: to prove are passed to the
> test(s) [0] so we could perhaps inspect @ARGV for the mode if we invent a new
> way to pass arguments.  Something along the lines of the untested sketch
> below in the pg_upgrade test:
> 
> +# Optionally set the file transfer mode for the tests via arguments to PROVE
> +my $mode = (@ARGV);
> +$mode = '--copy' unless defined;
> 
> .. together with an extension to Makefile.global.in ..
> 
> -   $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl)
> +   $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) $(if $(PROVE_TESTS),$(PROVE_TESTS),t/*.pl) $(PROVE_TEST_ARGS)
> 
> .. should *I think* allow for passing the mode to the tests via:
> 
> make -C src/bin/pg_upgrade check PROVE_TEST_ARGS=":: --link"

I think this might be a lot of complication to get working robustly and 
in the different build systems.  Plus, what happens if you run all the 
test suites and want to pass some options to pg_upgrade and some to 
another test?

I think if we want to make this configurable on the fly, and environment 
variable would be much easier, like

     my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';

On 07.12.22 17:33, Peter Eisentraut wrote:
> I think if we want to make this configurable on the fly, and environment 
> variable would be much easier, like
> 
>      my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';

Here is an updated patch set that incorporates this idea.

> On 14 Dec 2022, at 08:04, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>
> On 07.12.22 17:33, Peter Eisentraut wrote:
>> I think if we want to make this configurable on the fly, and environment variable would be much easier, like
>>     my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
>
> Here is an updated patch set that incorporates this idea.

I would prefer a small note about it in src/bin/pg_upgrade/TESTING to document
it outside of the code, but otherwise LGTM.

+        $mode,
         '--check'
     ],

...

-        '-p',         $oldnode->port,     '-P', $newnode->port
+        '-p',         $oldnode->port,     '-P', $newnode->port,
+        $mode,
     ],

Minor nitpick, but while in there should we take the opportunity to add a
trailing comma on the other two array declarations which now ends with --check?
It's good Perl practice and will make the code consistent.

--
Daniel Gustafsson        https://vmware.com/

At Wed, 14 Dec 2022 10:40:45 +0100, Daniel Gustafsson <daniel@yesql.se> wrote in 
> > On 14 Dec 2022, at 08:04, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
> > 
> > On 07.12.22 17:33, Peter Eisentraut wrote:
> >> I think if we want to make this configurable on the fly, and environment variable would be much easier, like
> >>     my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
> > 
> > Here is an updated patch set that incorporates this idea.

We have already PG_TEST_EXTRA. Shouldn't we use it here as well?

> I would prefer a small note about it in src/bin/pg_upgrade/TESTING to document
> it outside of the code, but otherwise LGTM.
> 
> +        $mode,
>          '--check'
>      ],
> 
> ...
> 
> -        '-p',         $oldnode->port,     '-P', $newnode->port
> +        '-p',         $oldnode->port,     '-P', $newnode->port,
> +        $mode,
>      ],
> 
> Minor nitpick, but while in there should we take the opportunity to add a
> trailing comma on the other two array declarations which now ends with --check?
> It's good Perl practice and will make the code consistent.

Otherwise look god to me.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

> On 15 Dec 2022, at 01:56, Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
>
> At Wed, 14 Dec 2022 10:40:45 +0100, Daniel Gustafsson <daniel@yesql.se> wrote in
>>> On 14 Dec 2022, at 08:04, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>>>
>>> On 07.12.22 17:33, Peter Eisentraut wrote:
>>>> I think if we want to make this configurable on the fly, and environment variable would be much easier, like
>>>>    my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
>>>
>>> Here is an updated patch set that incorporates this idea.
>
> We have already PG_TEST_EXTRA. Shouldn't we use it here as well?

I think those are two different things.  PG_TEST_EXTRA adds test suites that
aren't run by default, this proposal is to be able to inject options into a
test suite to modify its behavior.

--
Daniel Gustafsson        https://vmware.com/

On 14.12.22 10:40, Daniel Gustafsson wrote:
>> On 14 Dec 2022, at 08:04, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>>
>> On 07.12.22 17:33, Peter Eisentraut wrote:
>>> I think if we want to make this configurable on the fly, and environment variable would be much easier, like
>>>      my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
>>
>> Here is an updated patch set that incorporates this idea.
> 
> I would prefer a small note about it in src/bin/pg_upgrade/TESTING to document
> it outside of the code, but otherwise LGTM.
> 
> +        $mode,
>           '--check'
>       ],
> 
> ...
> 
> -        '-p',         $oldnode->port,     '-P', $newnode->port
> +        '-p',         $oldnode->port,     '-P', $newnode->port,
> +        $mode,
>       ],
> 
> Minor nitpick, but while in there should we take the opportunity to add a
> trailing comma on the other two array declarations which now ends with --check?
> It's good Perl practice and will make the code consistent.

committed with these changes

Hi, 
With the addition of --copy option, pg_upgrade now has three possible transfer mode options. Currently, an error does
notoccur even if multiple transfer modes are specified. For example, we can also run "pg_upgrade --link --copy --clone"
command.As discussed in Horiguchi-san's previous email, options like "--mode=link|copy|clone" can prevent this
problem.
The attached patch uses the current implementation and performs a minimum check to prevent multiple transfer modes from
beingspecified.
 

Regards,
Noriyoshi Shinoda
-----Original Message-----
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com> 
Sent: Saturday, December 17, 2022 2:44 AM
To: Daniel Gustafsson <daniel@yesql.se>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: pg_upgrade: Make testing different transfer modes easier

On 14.12.22 10:40, Daniel Gustafsson wrote:
>> On 14 Dec 2022, at 08:04, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
>>
>> On 07.12.22 17:33, Peter Eisentraut wrote:
>>> I think if we want to make this configurable on the fly, and environment variable would be much easier, like
>>>      my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
>>
>> Here is an updated patch set that incorporates this idea.
> 
> I would prefer a small note about it in src/bin/pg_upgrade/TESTING to 
> document it outside of the code, but otherwise LGTM.
> 
> +        $mode,
>           '--check'
>       ],
> 
> ...
> 
> -        '-p',         $oldnode->port,     '-P', $newnode->port
> +        '-p',         $oldnode->port,     '-P', $newnode->port,
> +        $mode,
>       ],
> 
> Minor nitpick, but while in there should we take the opportunity to 
> add a trailing comma on the other two array declarations which now ends with --check?
> It's good Perl practice and will make the code consistent.

committed with these changes

> On 19 Dec 2022, at 01:39, Shinoda, Noriyoshi (PN Japan FSIP) <noriyoshi.shinoda@hpe.com> wrote:

> With the addition of --copy option, pg_upgrade now has three possible transfer mode options. Currently, an error does
notoccur even if multiple transfer modes are specified. For example, we can also run "pg_upgrade --link --copy --clone"
command.As discussed in Horiguchi-san's previous email, options like "--mode=link|copy|clone" can prevent this problem. 
> The attached patch uses the current implementation and performs a minimum check to prevent multiple transfer modes
frombeing specified. 

We typically allow multiple invocations of the same parameter with a
last-one-wins strategy, and only error out when competing *different*
parameters are present.  A --mode=<string> parameter can still be added as
syntactic sugar, but multiple-choice parameters is not a commonly used pattern
in postgres utilities (pg_dump/restore and pg_basebackup are ones that come to
mind).

--
Daniel Gustafsson        https://vmware.com/