Обсуждение: Test 002_pg_upgrade fails with olddump on Windows

Hello,

When trying to use a custom dump with the test pg_upgrade/002_pg_upgrade,
I observe the following test failure on Windows:
 >meson test --suite setup
 >echo create database regression>...\dump.sql
 >set olddump=...\dump.sql& set oldinstall=.../tmp_install/usr/local/pgsql& meson test pg_upgrade/002_pg_upgrade

1/1 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade ERROR            11.38s   exit status 1

regress_log_002_pg_upgrade.txt contains:
...
[09:07:06.704](3.793s) ok 11 - run of pg_upgrade for new instance
...
[09:07:07.301](0.001s) not ok 15 - old and new dumps match after pg_upgrade
[09:07:07.301](0.000s) #   Failed test 'old and new dumps match after pg_upgrade'
#   at .../src/bin/pg_upgrade/t/002_pg_upgrade.pl line 452.
[09:07:07.301](0.000s) #          got: '1'
#     expected: '0'
=== diff of ...\build\testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8/dump1.sql and 
...\build\testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8/dump2.sql
=== stdout ===
=== stderr ===
=== EOF ===


 >dir "testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8/"
11/25/2023  09:06 AM             2,729 dump1.sql
11/25/2023  09:07 AM             2,590 dump2.sql

 >diff -s "testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8\dump1.sql" 
"testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8\dump2.sql"
Files testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8\dump1.sql and 
testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_ifk8\dump2.sql are identical

As I can see, dump1.sql contains line endings 0d 0a, while dump2.sql — 0a.

The attached patch fixes the issue for me.

Best regards,
Alexander

On Sat, Nov 25, 2023 at 11:00:01PM +0300, Alexander Lakhin wrote:
> diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
> index c6d83d3c21..d34b45e346 100644
> --- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
> +++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
> @@ -293,6 +293,7 @@ if (defined($ENV{oldinstall}))
>      }
>
>      open my $fh, ">", $dump1_file or die "could not open dump file";
> +    binmode $fh;
>      print $fh $dump_data;
>      close $fh;

There is something I don't get here.  The old and new dump files
should be processed in filter_dump(), where
AdjustUpgrade::adjust_old_dumpfile does the following so binmode
should not be needed:
        # use Unix newlines
        $dump =~ s/\r\n/\n/g;

Or you have used the test suite with an old installation that has the
same major version as the new installation, meaning that the filtering
was not happening, still you have detected some diffs?  It sounds to
me that we should just apply the filters to the dumps all the time if
you have used matching versions.  The filtering would remove only the
comments, some extra newlines and replace the CRLFs in this case.
--
Michael

Hi Michael,

05.12.2023 10:56, Michael Paquier wrote:

> Or you have used the test suite with an old installation that has the
> same major version as the new installation, meaning that the filtering
> was not happening, still you have detected some diffs?  It sounds to
> me that we should just apply the filters to the dumps all the time if
> you have used matching versions.  The filtering would remove only the
> comments, some extra newlines and replace the CRLFs in this case.

Yes, my case is with the same version, literally:
build>echo create database regression>c:\temp\dump.sql
build>set olddump=c:\temp\dump.sql& set oldinstall=%CD%/tmp_install/usr/local/pgsql& meson test
pg_upgrade/002_pg_upgrade

So removing the condition "if ($oldnode->pg_version != $newnode->pg_version)"
works here as well, but maybe switching the file mode (to preserve EOLs
produced by pg_dump) in the block "After dumping, update references ..."
is more efficient than filtering dumps (on all OSes?).

Best regards,
Alexander

On Tue, Dec 05, 2023 at 12:00:00PM +0300, Alexander Lakhin wrote:
> So removing the condition "if ($oldnode->pg_version != $newnode->pg_version)"
> works here as well, but maybe switching the file mode (to preserve EOLs
> produced by pg_dump) in the block "After dumping, update references ..."
> is more efficient than filtering dumps (on all OSes?).

Well, there's the argument that we replace the library references in
a SQL file that we are handling as a text file, so switching it to use
the binary mode is not right.  A second argument is to apply the same
filtering logic across both the old and new dumps, even if we know
that the second dump file taken by pg_dump with not append CRLFs.

At the end, just applying the filtering all the time makes the most
sense to me, so I've applied a patch doing just that.
--
Michael

06.12.2023 04:17, Michael Paquier wrote:
> At the end, just applying the filtering all the time makes the most
> sense to me, so I've applied a patch doing just that.

Thank you for the fix!

Now that test with the minimal dump passes fine, but when I tried to run
it with a complete dump borrowed from a normal test run:
set olddump=& set oldinstall=& set PG_TEST_NOCLEAN=1& meson test pg_upgrade/002_pg_upgrade
REM this test succeeded
copy testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_*\dump1.sql
set olddump=c:\temp\dump1.sql& set oldinstall=%CD%/tmp_install/usr/local/pgsql& meson test pg_upgrade/002_pg_upgrade

I encountered another failure:
...
Creating dump of database schemas                             ok
Checking for presence of required libraries                   fatal

Your installation references loadable libraries that are missing from the
new installation.  You can add these libraries to the new installation,
or remove the functions using them from the old installation.  A list of
problem libraries is in the file:

.../build/testrun/pg_upgrade/002_pg_upgrade/data/t_002_pg_upgrade_new_node_data/pgdata/pg_upgrade_output.d/20231205T223247.304/loadable_libraries.txt
Failure, exiting
[22:32:51.086](3.796s) not ok 11 - run of pg_upgrade for new instance
...

loadable_libraries.txt contains:
could not load library ".../src/test/regress/refint.dll": ERROR: could not access file 
".../src/test/regress/refint.dll": No such file or directory
In database: regression
could not load library ".../src/test/regress/autoinc.dll": ERROR: could not access file 
".../src/test/regress/autoinc.dll": No such file or directory
In database: regression
could not load library ".../src/test/regress/regress.dll": ERROR: could not access file 
".../src/test/regress/regress.dll": No such file or directory
In database: regression

Really, I can see refint.dll in ...\build\src\test\regress and in
...\build\tmp_install\usr\local\pgsql\lib, but not in
.../src/test/regress/regress.dll

c:\temp\dump1.sql contains:
...
CREATE FUNCTION public.check_primary_key() RETURNS trigger
     LANGUAGE c
     AS '.../build/src/test/regress/refint.dll', 'check_primary_key';

while ...\build\testrun\pg_upgrade\002_pg_upgrade\data\tmp_test_T6jE\dump1.sql
(for the failed test):
...
CREATE FUNCTION public.check_primary_key() RETURNS trigger
     LANGUAGE c
     AS '.../src/test/regress/refint.dll', 'check_primary_key';

The same is on Linux:
PG_TEST_NOCLEAN=1 meson test pg_upgrade/002_pg_upgrade
cp testrun/pg_upgrade/002_pg_upgrade/data/tmp_test_*/dump1.sql /tmp/
olddump=/tmp/dump1.sql oldinstall=`pwd`/tmp_install/usr/local/pgsql meson test pg_upgrade/002_pg_upgrade

So it looks like
     my $newregresssrc = "$srcdir/src/test/regress";
is incorrect for meson.
Maybe it should be?:
     my $newregresssrc = dirname($ENV{REGRESS_SHLIB});
(With this line the test passes for me on Windows and Linux).

Best regards,
Alexander

On Wed, Dec 06, 2023 at 11:00:01AM +0300, Alexander Lakhin wrote:
> So it looks like
>     my $newregresssrc = "$srcdir/src/test/regress";
> is incorrect for meson.
> Maybe it should be?:
>     my $newregresssrc = dirname($ENV{REGRESS_SHLIB});
> (With this line the test passes for me on Windows and Linux).

Hmm.  Yes, it looks like you're right here.  That should allow all the
scenarios we expect to work to update the paths for the functions.
--
Michael

On Thu, Dec 07, 2023 at 05:44:53PM +0900, Michael Paquier wrote:
> Hmm.  Yes, it looks like you're right here.  That should allow all the
> scenarios we expect to work to update the paths for the functions.

And done this one as well down to v15, where not only meson, but also
vpath could have been confused with an update to an incorrect path.
--
Michael

On 2023-Dec-08, Michael Paquier wrote:

> On Thu, Dec 07, 2023 at 05:44:53PM +0900, Michael Paquier wrote:
> > Hmm.  Yes, it looks like you're right here.  That should allow all the
> > scenarios we expect to work to update the paths for the functions.
> 
> And done this one as well down to v15, where not only meson, but also
> vpath could have been confused with an update to an incorrect path.

Argh, yeah, this has caused me pain a couple of times.  Thanks for fixing.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"If you have nothing to say, maybe you need just the right tool to help you
not say it."                   (New York Times, about Microsoft PowerPoint)