Re: Rewriting the test of pg_upgrade as a TAP test - take three - remastered set

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