On Sun, May 01, 2022 at 09:27:18PM -0700, Noah Misch wrote: > On Fri, Apr 01, 2022 at 10:16:48AM +0900, Michael Paquier wrote: >> commit 322becb wrote: > > Nothing checks the command result, so the test file passes even if each of > these createdb calls fails. Other run_log() calls in this file have the same > problem. This particular one should be command_ok() or similar.
All of them could rely on command_ok(), as they should never fail, so
switched this way.
> --host and --port are redundant in a PostgreSQL::Test::Cluster::run_log call,
> because that call puts equivalent configuration in the environment. Other
> calls in the file have the same redundant operands. (No other test file has
> redundant --host or --port.)
Right. Removed all that.
>> + # Grab any regression options that may be passed down by caller.
>> + my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";
>
> Typo: s/_OPT/_OPTS/
Oops, fixed.
>> + my @extra_opts = split(/\s+/, $extra_opts_val);
>
> src/test/recovery/t/027_stream_regress.pl and the makefiles treat
> EXTRA_REGRESS_OPTS as a shell fragment. To be compatible, use the
> src/test/recovery/t/027_stream_regress.pl approach. Affected usage patetrns
> are not very important, but since the tree has code for it, you may as well
> borrow that code. These examples witness the difference:
So the pattern of EXTRA_REGRESS_OPTS being used in the Makefiles is
the decision point here. Makes sense.
>> -# Create databases with names covering the ASCII bytes other than NUL, BEL,
>> -# LF, or CR. BEL would ring the terminal bell in the course of this test,
>> and
>> -# it is not otherwise a special case. PostgreSQL doesn't support the rest.
>> -dbname1=`awk 'BEGIN { for (i= 1; i < 46; i++)
>> - if (i != 7 && i != 10 && i != 13) printf "%c", i }' </dev/null`
>> -# Exercise backslashes adjacent to double quotes, a Windows special case.
>> -dbname1='\"\'$dbname1'\\"\\\'
>
> This rewrite dropped the exercise of backslashes adjacent to double quotes.
Damn, thanks. If I am reading that right, this could be done with the
following addition in generate_db(), adding double quotes surrounded
by backslashes before and after the database name:
$dbname = '\\"\\' . $dbname . '\\"\\';
All these fixes lead me to the attached patch. Does that look fine to
you?
Thanks,
--
Michael
diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 05bf161843..4002982dc0 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -21,9 +21,11 @@ sub generate_db
next if $i == 7 || $i == 10 || $i == 13; # skip BEL, LF, and CR
$dbname = $dbname . sprintf('%c', $i);
}
- $node->run_log(
- [ 'createdb', '--host', $node->host, '--port', $node->port, $dbname ]
- );
+
+ # Exercise backslashes adjacent to double quotes, a Windows special
+ # case.
+ $dbname = '\\"\\' . $dbname . '\\"\\';
+ $node->command_ok([ 'createdb', $dbname ]);
}
# The test of pg_upgrade requires two clusters, an old one and a new one
@@ -70,12 +72,7 @@ if (defined($ENV{olddump}))
# Load the dump using the "postgres" database as "regression" does
# not exist yet, and we are done here.
- $oldnode->command_ok(
- [
- 'psql', '-X', '-f', $olddumpfile,
- '--port', $oldnode->port, '--host', $oldnode->host,
- 'postgres'
- ]);
+ $oldnode->command_ok([ 'psql', '-X', '-f', $olddumpfile, 'postgres' ]);
}
else
{
@@ -87,8 +84,7 @@ else
generate_db($oldnode, 91, 127);
# Grab any regression options that may be passed down by caller.
- my $extra_opts_val = $ENV{EXTRA_REGRESS_OPT} || "";
- my @extra_opts = split(/\s+/, $extra_opts_val);
+ my $extra_opts = $ENV{EXTRA_REGRESS_OPTS} || "";
# --dlpath is needed to be able to find the location of regress.so
# and any libraries the regression tests require.
@@ -100,19 +96,19 @@ else
# --inputdir points to the path of the input files.
my $inputdir = "$srcdir/src/test/regress";
- my @regress_command = [
- $ENV{PG_REGRESS}, @extra_opts,
- '--dlpath', $dlpath,
- '--max-concurrent-tests', '20',
- '--bindir=', '--host',
- $oldnode->host, '--port',
- $oldnode->port, '--schedule',
- "$srcdir/src/test/regress/parallel_schedule", '--outputdir',
- $outputdir, '--inputdir',
- $inputdir
- ];
-
- my $rc = run_log(@regress_command);
+ my $rc =
+ system($ENV{PG_REGRESS}
+ . "$extra_opts "
+ . "--dlpath=\"$dlpath\" "
+ . "--bindir= "
+ . "--host="
+ . $oldnode->host . " "
+ . "--port="
+ . $oldnode->port . " "
+ . "--schedule=$srcdir/src/test/regress/parallel_schedule "
+ . "--max-concurrent-tests=20 "
+ . "--inputdir=\"$inputdir\" "
+ . "--outputdir=\"$outputdir\"");
if ($rc != 0)
{
# Dump out the regression diffs file, if there is one
@@ -133,12 +129,10 @@ if (defined($ENV{oldinstall}))
{
# Note that upgrade_adapt.sql from the new version is used, to
# cope with an upgrade to this version.
- $oldnode->run_log(
+ $oldnode->command_ok(
[
- 'psql', '-X',
- '-f', "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
- '--port', $oldnode->port,
- '--host', $oldnode->host,
+ 'psql', '-X',
+ '-f', "$srcdir/src/bin/pg_upgrade/upgrade_adapt.sql",
'regression'
]);
}
@@ -232,7 +226,7 @@ if (-d $log_path)
}
# Second dump from the upgraded instance.
-$newnode->run_log(
+$newnode->command_ok(
[
'pg_dumpall', '--no-sync',
'-d', $newnode->connstr('postgres'),
signature.asc
Description: PGP signature
