Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation

Masahiko Sawada <sawada.mshk@gmail.com> writes:

> On Tue, Feb 25, 2025 at 3:03 PM Dagfinn Ilmari Mannsåker
> <ilmari@ilmari.org> wrote:
>>
>> Hi,
>>
>> While working on another round of the long option and fat comma style
>> cleanup, I noticed that the test for pg_upgrade --set-char-signedess
>> doesn't test what it's supposed to:
>>
>> Masahiko Sawada <sawada.mshk@gmail.com> writes:
>>
>> > diff --git a/src/bin/pg_upgrade/t/005_char_signedness.pl b/src/bin/pg_upgrade/t/005_char_signedness.pl
>> > index 05c3014a27d..c024106863e 100644
>> > --- a/src/bin/pg_upgrade/t/005_char_signedness.pl
>> > +++ b/src/bin/pg_upgrade/t/005_char_signedness.pl
>> > @@ -40,6 +40,23 @@ command_like(
>> >       qr/Default char data signedness:\s+unsigned/,
>> >       'updated default char signedness is unsigned in control file');
>> >
>> > +# Cannot use --set-char-signedness option for upgrading from v18+
>> > +command_fails(
>> > +     [
>> > +             'pg_upgrade', '--no-sync',
>> > +             '-d', $old->data_dir,
>> > +             '-D', $new->data_dir,
>> > +             '-b', $old->config_data('--bindir'),
>> > +             '-B', $new->config_data('--bindir'),
>> > +             '-s', $new->host,
>> > +             '-p', $old->port,
>> > +             '-P', $new->port,
>> > +             '-set-char-signedness', 'signed',
>>
>> This is missing a dash, which causes the command to fail, but for the
>> wrong reason.  pg_uprade seems to print all its errors on stdout, which
>> I guess is why the test use plain command_fails() instead of
>> command_fails_like(). However, we have another function to deal with
>> this: command_checks_all().  Attached are patches that fix the above
>> test, and also convert the other command_fails() calls in the pg_upgrade
>> tests to test for specific messages.
>
> Thank you for the report. I agree with both points.
>
> I believe that replacing command_fails_like() with
> command_checks_all() is an improvement whereas adding the missing dash
> to --set-char-signendess option is a bug fix. How about reorganizing
> the patches as follows?
>
> - 0001 patch just adds the dash to the --set-char-signedness.
> - 0002 patch uses command_checks_all() in 002_pg_upgrade.pl,
> 004_subscription.pl, and 005_char_signedness.pl for checking the
> output better.

Yeah, that's a better way to organise it, updated patches attached.

- ilmari

From 0671eb2a6f59926bf94b041a1e20b864ec370a25 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 26 Feb 2025 10:51:28 +0000
Subject: [PATCH 1/2] pg_upgrade: fix spelling of --set-char-signedness in test

The single dash causes it to fail with a different error, but because
the test isn't checking the error message it was passing anyway.

The next commit will make the test check for the actual error.
---
 src/bin/pg_upgrade/t/005_char_signedness.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/pg_upgrade/t/005_char_signedness.pl b/src/bin/pg_upgrade/t/005_char_signedness.pl
index c024106863e..b3092f03bb7 100644
--- a/src/bin/pg_upgrade/t/005_char_signedness.pl
+++ b/src/bin/pg_upgrade/t/005_char_signedness.pl
@@ -51,7 +51,7 @@
         '-s', $new->host,
         '-p', $old->port,
         '-P', $new->port,
-        '-set-char-signedness', 'signed',
+        '--set-char-signedness', 'signed',
         $mode
     ],
     '--set-char-signedness option cannot be used for upgrading from v18 or later'
-- 
2.48.1

From 371f4fafc93e8520b4cb49a657ca56131da3b3e4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Wed, 26 Feb 2025 10:53:40 +0000
Subject: [PATCH 2/2] pg_upgrade: check for the expected error message in tests

pg_upgrade prints its error messages on stdout, so we can't use
command_fails_like() to check that it fails for the right reason.
Instead use command_checks_all() to check the exit status and stdout.
---
 src/bin/pg_upgrade/t/002_pg_upgrade.pl      | 5 ++++-
 src/bin/pg_upgrade/t/004_subscription.pl    | 8 +++++++-
 src/bin/pg_upgrade/t/005_char_signedness.pl | 5 ++++-
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/bin/pg_upgrade/t/002_pg_upgrade.pl b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
index 45ea94c84bb..cccba9dc3db 100644
--- a/src/bin/pg_upgrade/t/002_pg_upgrade.pl
+++ b/src/bin/pg_upgrade/t/002_pg_upgrade.pl
@@ -396,7 +396,7 @@ sub filter_dump
 # Cause a failure at the start of pg_upgrade, this should create the logging
 # directory pg_upgrade_output.d but leave it around.  Keep --check for an
 # early exit.
-command_fails(
+command_checks_all(
     [
         'pg_upgrade', '--no-sync',
         '-d', $oldnode->data_dir,
@@ -408,6 +408,9 @@ sub filter_dump
         '-P', $newnode->port,
         $mode, '--check',
     ],
+    1,
+    [qr{check for ".*?does.not.exist" failed:}],
+    [],
     'run of pg_upgrade --check for new instance with incorrect binary path');
 ok(-d $newnode->data_dir . "/pg_upgrade_output.d",
     "pg_upgrade_output.d/ not removed after pg_upgrade failure");
diff --git a/src/bin/pg_upgrade/t/004_subscription.pl b/src/bin/pg_upgrade/t/004_subscription.pl
index 13773316e1d..3f9734f7b3f 100644
--- a/src/bin/pg_upgrade/t/004_subscription.pl
+++ b/src/bin/pg_upgrade/t/004_subscription.pl
@@ -130,7 +130,7 @@
 
 $old_sub->stop;
 
-command_fails(
+command_checks_all(
     [
         'pg_upgrade',
         '--no-sync',
@@ -144,6 +144,12 @@
         $mode,
         '--check',
     ],
+    1,
+    [
+        qr{\QYour installation contains subscriptions without origin},
+        qr{\Qrelations not in i (initialize) or r (ready) state},
+    ],
+    [],
     'run of pg_upgrade --check for old instance with relation in \'d\' datasync(invalid) state and missing replication
origin'
 );
 
diff --git a/src/bin/pg_upgrade/t/005_char_signedness.pl b/src/bin/pg_upgrade/t/005_char_signedness.pl
index b3092f03bb7..d186822ac77 100644
--- a/src/bin/pg_upgrade/t/005_char_signedness.pl
+++ b/src/bin/pg_upgrade/t/005_char_signedness.pl
@@ -41,7 +41,7 @@
     'updated default char signedness is unsigned in control file');
 
 # Cannot use --set-char-signedness option for upgrading from v18+
-command_fails(
+command_checks_all(
     [
         'pg_upgrade', '--no-sync',
         '-d', $old->data_dir,
@@ -54,6 +54,9 @@
         '--set-char-signedness', 'signed',
         $mode
     ],
+    1,
+    [qr/--set-char-signedness option cannot be used/],
+    [],
     '--set-char-signedness option cannot be used for upgrading from v18 or later'
 );
 
-- 
2.48.1