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

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.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com