Date: Fri, 30 Aug 2024 20:10:38 -0700
From: Noah Misch <noah@leadboat.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Joe Conway <mail@joeconway.com>,
	Peter Eisentraut <peter@eisentraut.org>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	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
Message-ID: <20240831031038.31.nmisch@google.com>
References: <1685377.1714496091@sss.pgh.pa.us>
 <CAPpHfdsgjG1nQoMjoUH5R6NE9-tE3=i6pPJNQiqDgQuHW-AHdA@mail.gmail.com>
 <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAD21AoBO-iL=UgHKQrKE=7zBrMYR_fAhLb8AGo_kmNgmkPKuQA@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: <https://www.postgresql.org/message-id/20240831031038.31.nmisch%40google.com>
Precedence: bulk

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@leadboat.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".)

> dynamically use the correct compare function for the same operator
> class depending on the char signedness. But is it possible to do it on
> the extension (e.g. pg_trgm) side?

No, I don't think the extension can do that cleanly.  It would need to store
the signedness in the index somehow, and GIN doesn't call the opclass at a
time facilitating that.  That would need help from the core server.