MIME-Version: 1.0
References: 
 <CAEze2WhsqYjg0oGY+7yooimUK7zRc9PY9u8u-Oo=VmJ+DAAkKg@mail.gmail.com>
 <CACJufxFV5KqmF86upFmX2tPzfKMWjWNf8+uYiYufLKatOsYXQw@mail.gmail.com>
 <CAEze2Wg4Fj+9LQOv=J5cqwapD8vz3oDh1B0iyX7RwesvseH9gg@mail.gmail.com>
 <CAEze2WjUyYhcUwzaPiQbe-xBB_knbnG8Xr_9ACL7HrGVx=Vydw@mail.gmail.com>
In-Reply-To: 
 <CAEze2WjUyYhcUwzaPiQbe-xBB_knbnG8Xr_9ACL7HrGVx=Vydw@mail.gmail.com>
From: David Rowley <dgrowleyml@gmail.com>
Date: Thu, 26 Mar 2026 09:40:08 +1300
Message-ID: 
 <CAApHDvpC8sj-ayXMQsh5DfnZV+X2QtUdvQ3Qr=81bcyaW7gVFw@mail.gmail.com>
Subject: Re: SQL-level pg_datum_image_equal
To: Matthias van de Meent <boekewurm+postgres@gmail.com>
Cc: jian he <jian.universality@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAApHDvpC8sj-ayXMQsh5DfnZV%2BX2QtUdvQ3Qr%3D81bcyaW7gVFw%40mail.gmail.com>
Precedence: bulk

On Thu, 26 Mar 2026 at 03:46, Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:
> Attached is v2, which adds the new function to the docs, in addition
> to rebasing the patch onto master.

In [1] I was looking into reported bugs with the datum_image code not
behaving correctly. It turns out this is due to hash_numeric() using
the wrong PG_RETURN_* macro, resulting in it not correctly
sign-extending negative 32-bit ints. I did some analysis to see if
there were any user-visible changes from fixing that in the back
branches. I didn't find any.  What you're adding here would add one. I
think we might need to become more strict about using the correct
return macro before we expose this stuff, else it's going to be a
rather scary process to fix bugs when it could affect the
datum_image_equal result.

The thing about Memoize slowly finding all the bugs in the
datum_image_* code is that we're free to fix these bugs, as the use
case (the Memoize hash table) only lasts as long as the query.  If we
expose all of this to users, then they could persist things in tables.
6911f8037 fixed another problem that resulted in the datum_image code
not doing the right thing.

I also proposed a hack in [2] to fix the sign-extension problem. I had
hoped that might be temporary, but if we were to expose this function
at the SQL level, then I doubt we'd be brave enough to remove it.

Consider the results of the following 2 queries, which only differ in
that one uses a materialized CTE and the other does not.

with cte(hash) as materialized (select hash_numeric(n) from
(values('1234.124'::numeric),('1234.124'::numeric)) n(n))
select * from cte where pg_datum_image_equal(hash,
hash_numeric('1234.124'::numeric)); -- wrong results

with cte(hash) as (select hash_numeric(n) from
(values('1234.124'::numeric),('1234.124'::numeric)) n(n))
select * from cte where pg_datum_image_equal(hash,
hash_numeric('1234.124'::numeric));

 hash
------
(0 rows)

    hash
------------
 -612512148
 -612512148
(2 rows)

This would work correctly if I pushed the patch in [2]. But as of yet,
I'm not sure about it. I at least think we should delay doing this
until we've come up with a fix that we're confident in.

David

[1] https://postgr.es/m/CAApHDvrRR1VOk4i4CpNWeL48veFshfRAvDTuWxsiUhUqo0akwQ%40mail.gmail.com
[2] https://postgr.es/m/CAApHDvreF-UiqBaHtRTQWQ6z1X9snstJW%2Bdfb2DU5GOb-uPEBg%40mail.gmail.com