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

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.

- ilmari