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

On Wed, Sep 18, 2024 at 6:46 PM Noah Misch <noah(at)leadboat(dot)com> wrote:
>
> On Tue, Sep 17, 2024 at 09:43:41PM -0700, Masahiko Sawada wrote:
> > On Mon, Sep 16, 2024 at 9:24 AM Noah Misch <noah(at)leadboat(dot)com> wrote:
> > > On Thu, Sep 12, 2024 at 03:42:48PM -0700, Masahiko Sawada wrote:
> > > > On Tue, Sep 10, 2024 at 3:05 PM Noah Misch <noah(at)leadboat(dot)com> wrote:
> > > > > On Tue, Sep 10, 2024 at 05:56:47PM -0400, Tom Lane wrote:
> > > > > > Got it. So now I'm wondering if we need all the complexity of storing
> > > > > > stuff in the GIN metapages. Could we simply read the (primary's)
> > > > > > signedness out of pg_control and use that?
> > >
> > > > I've attached a PoC patch for this idea. We write the default char
> > > > signedness to the control file at initdb time. Then when comparing two
> > > > trgms, pg_trgm opclasses use a comparison function based on the char
> > > > signedness of the cluster. I've confirmed that the patch fixes the
> > > > reported case at least.
> > >
> > > I agree that proves the concept.
> >
> > Thanks. I like the simplicity of this approach. If we agree with this
> > approach, I'd like to proceed with it.
>
> Works for me.
>
> > Regardless of what approach we take, I wanted to provide some
> > regression tests for these changes, but I could not come up with a
> > reasonable idea. It would be great if we could do something like
> > 027_stream_regress.pl on cross-architecture replication. But just
> > executing 027_stream_regress.pl on cross-architecture replication
> > could not be sufficient since we would like to confirm query results
> > as well. If we could have a reasonable tool or way, it would also help
> > find other similar bugs related architectural differences.
>
> Perhaps add a regress.c function that changes the control file flag and
> flushes the change to disk?

I've tried this idea but found out that extensions can flush the
controlfile on the disk after flipping the char signedness value, they
cannot update the controlfile data on the shared memory. And, when the
server shuts down, the on-disk controlfile is updated with the
on-memory controlfile data, so the char signedness is reverted.

We can add a function to set the char signedness of on-memory
controlfile data or add a new option to pg_resetwal to change the char
signedness on-disk controlfile data but they seem to be overkill to me
and I'm concerned about misusing these option and function.

I've updated and splitted patches. They don't have regression tests yet.

Regards,

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