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

On Sat, May 4, 2024 at 7:36 AM Joe Conway <mail@joeconway.com> wrote:
>
> On 5/3/24 11:44, Peter Eisentraut wrote:
> > On 03.05.24 16:13, Tom Lane wrote:
> >> Peter Eisentraut <peter@eisentraut.org> writes:
> >>> On 30.04.24 19:29, Tom Lane wrote:
> >>>> Also, the bigger picture here is the seeming assumption that "if
> >>>> we change pg_trgm then it will be safe to replicate from x86 to
> >>>> arm".  I don't believe that that's a good idea and I'm unwilling
> >>>> to promise that it will work, regardless of what we do about
> >>>> char signedness.  That being the case, I don't want to invest a
> >>>> lot of effort in the signedness issue.  Option (1) is clearly
> >>>> a small change with little if any risk of future breakage.
> >>
> >>> But note that option 1 would prevent some replication that is currently
> >>> working.
> >>
> >> The point of this thread though is that it's working only for small
> >> values of "work".  People are rightfully unhappy if it seems to work
> >> and then later they get bitten by compatibility problems.
> >>
> >> Treating char signedness as a machine property in pg_control would
> >> signal that we don't intend to make it work, and would ensure that
> >> even the most minimal testing would find out that it doesn't work.
> >>
> >> If we do not do that, it seems to me we have to buy into making
> >> it work.  That would mean dealing with the consequences of an
> >> incompatible change in pg_trgm indexes, and then going through
> >> the same dance again the next time(s) similar problems are found.
> >
> > Yes, that is understood.  But anecdotally, replicating between x86-64 arm64 is
> > occasionally used for upgrades or migrations.  In practice, this appears to have
> > mostly worked.  If we now discover that it won't work with certain index
> > extension modules, it's usable for most users. Even if we say, you have to
> > reindex everything afterwards, it's probably still useful for these scenarios.
>
> +1

+1

How about extending amcheck to support GIN and GIst indexes so that it
can detect potential data incompatibility due to changing 'char' to
'unsigned char'? I think these new tests would be useful also for
users to check if they really need to reindex indexes due to such
changes. Also we fix pg_trgm so that it uses 'unsigned char' in PG18.
Users who upgraded to PG18 can run the new amcheck tests on the
primary as well as the standby.

Regards,

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