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

On Fri, Sep 06, 2024 at 02:07:10PM -0700, Masahiko Sawada wrote:
> On Fri, Aug 30, 2024 at 8:10 PM Noah Misch <noah(at)leadboat(dot)com> wrote:
> > On Thu, Aug 29, 2024 at 03:48:53PM -0500, Masahiko Sawada wrote:
> > > On Sun, May 19, 2024 at 6:46 AM Noah Misch <noah(at)leadboat(dot)com> wrote:
> > > > If I were standardizing pg_trgm on one or the other notion of "char", I would
> > > > choose signed char, since I think it's still the majority. More broadly, I
> > > > see these options to fix pg_trgm:
> > > >
> > > > 1. Change to signed char. Every arm64 system needs to scan pg_trgm indexes.
> > > > 2. Change to unsigned char. Every x86 system needs to scan pg_trgm indexes.
> > >
> > > Even though it's true that signed char systems are the majority, it
> > > would not be acceptable to force the need to scan pg_trgm indexes on
> > > unsigned char systems.
> > >
> > > > 3. Offer both, as an upgrade path. For example, pg_trgm could have separate
> > > > operator classes gin_trgm_ops and gin_trgm_ops_unsigned. Running
> > > > pg_upgrade on an unsigned-char system would automatically map v17
> > > > gin_trgm_ops to v18 gin_trgm_ops_unsigned. This avoids penalizing any
> > > > architecture with upgrade-time scans.
> > >
> > > Very interesting idea. How can new v18 users use the correct operator
> > > class? I don't want to require users to specify the correct signed or
> > > unsigned operator classes when creating a GIN index. Maybe we need to
> >
> > In brief, it wouldn't matter which operator class new v18 indexes use. The
> > documentation would focus on gin_trgm_ops and also say something like:
> >
> > There's an additional operator class, gin_trgm_ops_unsigned. It behaves
> > exactly like gin_trgm_ops, but it uses a deprecated on-disk representation.
> > Use gin_trgm_ops in new indexes, but there's no disadvantage from continuing
> > to use gin_trgm_ops_unsigned. Before PostgreSQL 18, gin_trgm_ops used a
> > platform-dependent representation. pg_upgrade automatically uses
> > gin_trgm_ops_unsigned when upgrading from source data that used the
> > deprecated representation.
> >
> > What concerns might users have, then? (Neither operator class would use plain
> > "char" in a context that affects on-disk state. They'll use "signed char" and
> > "unsigned char".)
>
> I think I understand your idea now. Since gin_trgm_ops will use
> "signed char", there is no impact for v18 users -- they can continue
> using gin_trgm_ops.

Right.

> But how does pg_upgrade use gin_trgm_ops_unsigned? This opclass will
> be created by executing the pg_trgm script for v18, but it isn't
> executed during pg_upgrade. Another way would be to do these opclass
> replacement when executing the pg_trgm's update script (i.e., 1.6 to
> 1.7).

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 "unsigned
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_unsigned,
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 first.
(In back branches, the C code behind gin_trgm_ops_unsigned could just raise
an error if called.)

What other options exist? I bet there are more.