pg_upgrade: Make testing different transfer modes easier

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 behavior by a configuration file or an environment variable, etc. But I don't mind the command had the effetively nop option only 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 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.)

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]https://perldoc.perl.org/prove#Arguments-to-Tests 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 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.)

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 not occur 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 being specified.

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 not occur 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 being 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/