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

On Wed, Feb 26, 2025 at 3:12 AM Dagfinn Ilmari Mannsåker
<ilmari(at)ilmari(dot)org> wrote:
>
> Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> writes:
>
> > On Tue, Feb 25, 2025 at 3:03 PM Dagfinn Ilmari Mannsåker
> > <ilmari(at)ilmari(dot)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(dot)mshk(at)gmail(dot)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.

Thank you for the patches.

The 0001 patch already got committed (945a9e38). I've simplified the
tests and updated the commit message for the 0002 patch. I'm going to
push it, barring further comments.

Regards,

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