Re: Rewriting the test of pg_upgrade as a TAP test - take two

On Thu, Mar 01, 2018 at 10:08:06PM -0800, Andres Freund wrote:
> On 2018-01-26 17:00:26 +0900, Michael Paquier wrote:
>> +elsif (defined($ENV{oldsrc}) &&
>> + defined($ENV{oldbindir}) &&
>> + defined($ENV{oldlibdir}))
>> +{
>> + # A run is wanted on an old version as base.
>> + $oldsrc = $ENV{oldsrc};
>> + $oldbindir = $ENV{oldbindir};
>> + $oldlibdir = $ENV{oldlibdir};
>> + # FIXME: this needs better tuning. Using "." or "$oldlibdir/postgresql"
>> + # causes the regression tests to pass but pg_upgrade to fail afterwards.
>
> Planning to fix it?

This is something I am not completely sure yet how to move on with.
Those areas would need adjustment once we ave a better idea what the
buildfarm can make use of.

>> +# Run regression tests on the old instance, using the binaries of this
>> +# instance. At the same time create a tablespace path needed for the
>> +# tests, similarly to what "make check" creates.
>
> What does "using binaries of this instance" mean? And why?

This refers to the installation of the instance to upgrade, including
pg_regress installed in pgxs.

>> +# Before dumping, get rid of objects not existing in later versions. This
>> +# depends on the version of the old server used, and matters only if the
>> +# old and new source paths
>> +my $oldpgversion;
>> +($result, $oldpgversion, $stderr) =
>> + $oldnode->psql('postgres', qq[SHOW server_version_num;]);
>> +my $fix_sql;
>> +if ($newsrc ne $oldsrc)
>> +{
>> + if ($oldpgversion <= 80400)
>> + {
>> + $fix_sql = "DROP FUNCTION public.myfunc(integer); DROP FUNCTION public.oldstyle_length(integer, text);";
>> + }
>> + else
>> + {
>> + $fix_sql = "DROP FUNCTION public.oldstyle_length(integer, text);";
>> + }
>> + $oldnode->psql('postgres', $fix_sql);
>> +}
>
> I know you copied this, but what?

Doesn't it matter to be able to test cross upgrades where the instance
to upgrade is 8.4?

>> +# Take a dump before performing the upgrade as a base comparison. Note
>> +# that we need to use pg_dumpall from PATH here.
>
> Whe do we need to?

Yeah, this should refer to the pg_dumpall command from the new
instance.
--
Michael