Re: pg_upgrade: Make testing different transfer modes easier
> On 19 Dec 2022, at 01:39, Shinoda, Noriyoshi (PN Japan FSIP) > 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= 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/
RE: pg_upgrade: Make testing different transfer modes easier
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
Sent: Saturday, December 17, 2022 2:44 AM
To: Daniel Gustafsson
Cc: PostgreSQL Hackers
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
>> 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
pg_upgrade_check_mode_v1.diff
Description: pg_upgrade_check_mode_v1.diff
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
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
Re: pg_upgrade: Make testing different transfer modes easier
> On 15 Dec 2022, at 01:56, Kyotaro Horiguchi wrote:
>
> At Wed, 14 Dec 2022 10:40:45 +0100, Daniel Gustafsson wrote
> in
>>> On 14 Dec 2022, at 08:04, Peter Eisentraut
>>> 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/
Re: pg_upgrade: Make testing different transfer modes easier
At Wed, 14 Dec 2022 10:40:45 +0100, Daniel Gustafsson wrote
in
> > On 14 Dec 2022, at 08:04, Peter Eisentraut
> > 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
Re: pg_upgrade: Make testing different transfer modes easier
> On 14 Dec 2022, at 08:04, Peter Eisentraut
> 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/
Re: pg_upgrade: Make testing different transfer modes easier
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.From c0f72bb9f50a36bc158943f3a51fbbc749d7f93c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Wed, 14 Dec 2022 07:52:58 +0100
Subject: [PATCH v2 1/2] pg_upgrade: Add --copy option
This option selects the default transfer mode. Having an explicit
option is handy to make scripts and tests more explicit. It also
makes it easier to talk about a "copy" mode rather than "the default
mode" or something like that, since until now the default mode didn't
have an externally visible name.
Discussion:
https://www.postgresql.org/message-id/flat/50a97009-8ff9-ca4d-a0f6-6086a6775a5b%40enterprisedb.com
---
doc/src/sgml/ref/pgupgrade.sgml | 10 ++
src/bin/pg_upgrade/option.c | 6 ++
2 files changed, 16 insertions(+)
diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
index 8f7a3025c3..7816b4c685 100644
--- a/doc/src/sgml/ref/pgupgrade.sgml
+++ b/doc/src/sgml/ref/pgupgrade.sgml
@@ -230,6 +230,16 @@ Options
+
+ --copy
+
+
+Copy files to the new cluster. This is the default. (See also
+--link and --clone.)
+
+
+
+
-?
--help
diff --git a/src/bin/pg_upgrade/option.c b/src/bin/pg_upgrade/option.c
index 2939f584b4..51fc7aede0 100644
--- a/src/bin/pg_upgrade/option.c
+++ b/src/bin/pg_upgrade/option.c
@@ -56,6 +56,7 @@ parseCommandLine(int argc, char *argv[])
{"socketdir", required_argument, NULL, 's'},
{"verbose", no_argument, NULL, 'v'},
{"clone", no_argument, NULL, 1},
+ {"copy", no_argument, NULL, 2},
{NULL, 0, NULL, 0}
};
@@ -194,6 +195,10 @@ parseCommandLine(int argc, char *argv[])
user_opts.transfer_mode = TRANSFER_MODE_CLONE;
break;
+ case 2:
+ user_opts.transfer_mode = TRANSFER_MODE_COPY;
+ break;
+
default:
fprintf(stderr, _("Try \"%s --help\" for more
information.\n"),
os_info.progname);
@@ -283,6 +288,7 @@ usage(void)
printf(_(" -v, --verbose enable verbose internal
logging\n"));
printf(_(" -V, --version display version information,
then exit\n"));
printf(_(" --clone clone instead of copying
files to new cluster\n"));
+ printf(_(" --copycopy files to new cluster
(default)\n"));
printf(_(" -?, --helpshow this help, then
exit\n"));
printf(_("\n"
"Before running pg_upgrade you must:\n"
base-commit: 60684dd834a222fefedd49b19d1f0a6189c1632e
--
2.38.1
From 49861815c1cf76658a77cf8ade59c4bb61c4064b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Wed, 14 Dec 2022 08:02:57 +0100
Subject: [PATCH v2 2/2] pg_upgrade: Make testing different transfer modes
easier
The environment variable PG_TEST_PG_UPGRADE_MODE can be set to
override the default transfer mode for the pg_upgrade tests.
(Automatically running the pg_upgrade tests for all supported modes
would be too slow.)
Discussion:
https://www.postgresql.org/message-id/flat/50a97009-8ff9-ca4d-a0f6-6086a6775a5b%40enterprisedb.com
---
src/bin/pg_upgrade/t/002_pg_upgrade.pl | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index add6ea9c34..1d5c80907c 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -12,6 +12,9 @@
use PostgreSQL::Test::Utils;
use Test::More;
+# Can be changed to test the other modes.
+my $mode = $ENV{PG_TEST_PG_UPGRADE_MODE} || '--copy';
+
# Generate a database with a name made of a range of ASCII characters.
sub generate_db
{
@@ -75,6 +78,8 @@ sub filter_dump
my $dump1_file = "$tempdir/dump1.sql";
my $dump2_file = "$tempdir/dump2.sql";
+note "testing using transfer mode $mode";
+
# Initialize node to upgrade
my $oldnode =
PostgreSQL::Test::Cluster->new('old_node',
@@ -128,6 +133,7 @@ sub filter_dump
# --inputdir points to the path of the input files.
my $inputdir = "$srcdir/src/test/regress";
+ note 'running regression tests in old instance';
my $rc =
system($ENV{PG_REGRESS}
. " $extra_opts "
@@ -256,6 +262,7 @@ sub filter_dump
'-s', $newnode->host,
'-p', $oldnode->port,
'-P', $newnode->port,
+ $mode,
Re: pg_upgrade: Make testing different transfer modes easier
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';
Re: pg_upgrade: Make testing different transfer modes easier
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= 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.
Re: pg_upgrade: Make testing different transfer modes easier
> On 1 Dec 2022, at 16:18, Peter Eisentraut
> 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] 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
Re: pg_upgrade: Make testing different transfer modes easier
At Thu, 1 Dec 2022 16:18:21 +0100, Peter Eisentraut 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= 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
