From: Tom Lane <tgl@sss.pgh.pa.us>
To: Noah Misch <noah@leadboat.com>
cc: Masahiko Sawada <sawada.mshk@gmail.com>, Joe Conway <mail@joeconway.com>,
        Peter Eisentraut <peter@eisentraut.org>,
        Alexander Korotkov <aekorotkov@gmail.com>,
        "Guo,
    Adam" <adamguo@amazon.com>,
        "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
        Nathan Bossart <nathandbossart@gmail.com>,
        Jim Mlodgenski <jimmy76@gmail.com>
Subject: Re: pg_trgm comparison bug on cross-architecture replication due to different char implementation
In-reply-to: <20240906215937.f7.nmisch@google.com>
References: <1696557.1714498172@sss.pgh.pa.us> <c15efbd9-fb92-4e72-8ffa-20dccf5515ad@eisentraut.org> <2652929.1714745623@sss.pgh.pa.us> <3846ef0f-5273-41aa-a77e-f92841e0515f@eisentraut.org> <362570f3-1031-4f8e-a077-81c103200b64@joeconway.com> <CAD21AoDJEKZO_=szRtOTX6we6wC_wWsHXH1zcEqza2vHE-YW4Q@mail.gmail.com> <20240518214546.e8@rfd.leadboat.com> <CAD21AoBO-iL=UgHKQrKE=7zBrMYR_fAhLb8AGo_kmNgmkPKuQA@mail.gmail.com> <20240831031038.31.nmisch@google.com> <CAD21AoC=7oF29N6iKaTssn-rRGnoTVSLzTQh4ia4ZUO1iKKc_w@mail.gmail.com> <20240906215937.f7.nmisch@google.com>
Comments: In-reply-to Noah Misch <noah@leadboat.com>
	message dated "Fri, 06 Sep 2024 14:59:37 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2385725.1725662213.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 06 Sep 2024 18:36:53 -0400
Message-ID: <2385726.1725662213@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/2385726.1725662213%40sss.pgh.pa.us>
Precedence: bulk

Noah Misch <noah@leadboat.com> writes:
> Yes, that's one way to make it work.  If we do it that way, it would be
> critical to make the ALTER EXTENSION UPDATE run before anything uses the
> index.  Otherwise, we'll run new v18 "signed char" code on a v17 "unsign=
ed
> char" data file.  Running ALTER EXTENSION UPDATE early enough should be
> feasible, so that's fine.  Some other options:

> - If v18 "pg_dump -b" decides to emit CREATE INDEX ... gin_trgm_ops_unsi=
gned,
>   then make it also emit the statements to create the opclass.

> - Ship pg_trgm--1.6--1.7.sql in back branches.  If the upgrade wants to =
use
>   gin_trgm_ops_unsigned, require the user to ALTER EXTENSION UPDATE firs=
t.
>   (In back branches, the C code behind gin_trgm_ops_unsigned could just =
raise
>   an error if called.)

I feel like all of these are leaning heavily on users to get it right,
and therefore have a significant chance of breaking use-cases that
were perfectly fine before.

Remind me of why we can't do something like this:

* Add APIs that allow opclasses to read/write some space in the GIN
metapage.  (We could get ambitious and add such APIs for other AMs
too, but doing it just for GIN is probably a prudent start.)  Ensure
that this space is initially zeroed.

* In gin_trgm_ops, read a byte of this space and interpret it as
	0 =3D unset
	1 =3D signed chars
	2 =3D unsigned chars
If the value is zero, set the byte on the basis of the native
char-signedness of the current platform.  (Obviously, a
secondary server couldn't write the byte, and would have to
wait for the primary to update the value.  In the meantime,
it's no more broken than today.)

* Subsequently, use either signed or unsigned comparisons
based on that value.

This would automatically do the right thing in all cases that
work today, and it'd provide the ability for cross-platform
replication to work in future.  You can envision cases where
transferring a pre-existing index to a platform of the other
stripe would misbehave, but they're the same cases that fail
today, and the fix remains the same: reindex.

			regards, tom lane