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>
 <CAApHDvpC8sj-ayXMQsh5DfnZV+X2QtUdvQ3Qr=81bcyaW7gVFw@mail.gmail.com>
In-Reply-To: 
 <CAApHDvpC8sj-ayXMQsh5DfnZV+X2QtUdvQ3Qr=81bcyaW7gVFw@mail.gmail.com>
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Wed, 25 Mar 2026 22:25:33 +0100
Message-ID: 
 <CAEze2WhK4ko6YTVV10iVo8sna0s=pdFYJ2xPrNNwxc9PqJJZoA@mail.gmail.com>
Subject: Re: SQL-level pg_datum_image_equal
To: David Rowley <dgrowleyml@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/CAEze2WhK4ko6YTVV10iVo8sna0s%3DpdFYJ2xPrNNwxc9PqJJZoA%40mail.gmail.com>
Precedence: bulk

On Wed, 25 Mar 2026 at 21:40, David Rowley <dgrowleyml@gmail.com> wrote:
>
> 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.

Thank you for refering me to this.

> 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.

I'm not sure we can reliably enforce that sign extension is always
going to be consistent for the same type; specifically looking at
unsigned integers as example. PG's tuple deserialization would see 4
byte alignment and sign-extend it as if it were a signed integer, but
returned values would prefer non- sign-extended values for more
efficient casts from larger unsigned types.

> 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'm happy to mark this function as STABLE for now (to prevent its
inclusion in permanent storage), and/or to adjust the code to adjust
for subnormal inputs (values with incorrect/inconsistent/unexpected
sign extensions).

> 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.

pg_datum_image_equal is supposed to only care about the bytes of the
datum that contain the actual value, so I can surely update it to do
that. I'll be happy to only apply that part to the code inside
pg_datum_image_equal; that should remove the part that exposes the
issue directly to users.

> 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.

I'm happy with any of the options: Having your patch at [2] get
committed, me applying [2]'s changes in pg_datum_image_equal, or
marking the function STABLE (and so, disallowing using its output in
permanent storage).

Note that false negatives are unlikely to be a problem, whilst false
positives might have issues. Sign extension issues here would create
false negatives, so likely wouldn't be a huge problem.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

PS. isn't this an issue with OIDs getting read from disk vs copied
around in memory, too? Oids are unsigned, whilst the deserialization
path assumes signed, so I'd expect that to have different outputs when
the from-disk OID you're checking has its not-sign bit 31 set?

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