Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation - Mailing list pgsql-hackers

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.

- ilmari

From b80c653c6635096345ec453bfe9445a82b7ab049 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Tue, 25 Feb 2025 22:27:36 +0000
Subject: [PATCH 1/2] pg_upgrade: fix --set-char-signedness failure test

The test had the option misspelled with only one dash, which fails, but
not with the expected message. Since pg_upgrade gives its error messages
on stdout(?!), use command_checks_all() instead of command_fails_like().
---
 src/bin/pg_upgrade/t/005_char_signedness.pl | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/bin/pg_upgrade/t/005_char_signedness.pl b/src/bin/pg_upgrade/t/005_char_signedness.pl
index c024106863e..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 @@ command_like(
     '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,
@@ -51,9 +51,12 @@ command_fails(
         '-s', $new->host,
         '-p', $old->port,
         '-P', $new->port,
-        '-set-char-signedness', 'signed',
+        '--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.43.0

From 03d25df987029da7511ff5a6f4ce69d1aef16ea9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Tue, 25 Feb 2025 22:56:12 +0000
Subject: [PATCH 2/2] pg_upgrade: use command_checks_all() to check for errors

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, we have to use command_checks_all(), which this patch does.
---
 src/bin/pg_upgrade/t/002_pg_upgrade.pl   | 5 ++++-
 src/bin/pg_upgrade/t/004_subscription.pl | 8 +++++++-
 2 files changed, 11 insertions(+), 2 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 @@ $oldnode->stop;
 # 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 @@ command_fails(
         '-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->safe_psql('postgres',
 
 $old_sub->stop;
 
-command_fails(
+command_checks_all(
     [
         'pg_upgrade',
         '--no-sync',
@@ -144,6 +144,12 @@ command_fails(
         $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'
 );
 
-- 
2.43.0