[PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support

On Wed, Jun 03, 2026 at 06:37:09PM -0500, Baji Shaik wrote:

int2 uses ssup_datum_int32_cmp because there is no int16-specific
helper, every int16 fits losslessly in int32, and int32_cmp is more
efficient than signed_cmp (4-byte radix passes instead of 8).

That's nice for such a simple change. That seems correct to me.
Could you add that to the next commit fest please at [1]https://commitfest.postgresql.org/59/ -- Michael?

Other custom fastcmp users in core (float4/float8, varlena types)
cannot be trivially switched due to NaN handling or locale-dependent
comparison, so they are left as-is.

Nope, we cannot do that.

[1]: https://commitfest.postgresql.org/59/ -- Michael
--
Michael

On Thu, Jun 4, 2026 at 10:45 PM Michael Paquier <michael@paquier.xyz> wrote:

That's nice for such a simple change. That seems correct to me.
Could you add that to the next commit fest please at [1]?

Thanks for the review.
Added to the commitfest: https://commitfest.postgresql.org/patch/6851/

On Thu, Jun 4, 2026 at 6:37 AM Baji Shaik <baji.pgdev@gmail.com> wrote:

The patch just replaces the comparator assignment and removes the
now-unused local fastcmp functions. No behavioral change and the
helpers produce identical results.

Pushed, thanks for the patch!

--
John Naylor
Amazon Web Services

Hello

I think this commit caused a regression with OIDs >= 2**31:

CREATE TABLE t (o oid);
INSERT INTO t VALUES ('2147483648');
SELECT o FROM t UNION ALL SELECT '3000000000'::oid ORDER BY 1;

On Thu, Jul 23, 2026 at 12:26 PM Zsolt Parragi
<zsolt.parragi@percona.com> wrote:

I think this commit caused a regression with OIDs >= 2**31:

CREATE TABLE t (o oid);
INSERT INTO t VALUES ('2147483648');
SELECT o FROM t UNION ALL SELECT '3000000000'::oid ORDER BY 1;

Thanks for reporting! Here's the problem: An oid from a heap tuple
goes through fetch_att(), which uses Int32GetDatum for all 4-byte
byval types, which sign-extends. An oid produced by oidin() goes
through ObjectIdGetDatum, which zero-extends. That didn't matter for
btoidfastcmp(), since it compared via DatumGetObjectId(x).

I think the easiest fix is to revert the oid part of commit 51cd5d6f0,
leaving behind the int2 and oid8 parts. The asymmetry between the 2
oid types would look odd, though, so that would require an explanatory
comment.

--
John Naylor
Amazon Web Services

I think the easiest fix is to revert the oid part of commit 51cd5d6f0,
leaving behind the int2 and oid8 parts. The asymmetry between the 2
oid types would look odd, though, so that would require an explanatory
comment.

Yes, that was my conclusion too, but I wanted to leave the decision up to you and Baji Shaik.

On Thu, Jul 23, 2026 at 3:15 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

I think the easiest fix is to revert the oid part of commit 51cd5d6f0,
leaving behind the int2 and oid8 parts. The asymmetry between the 2
oid types would look odd, though, so that would require an explanatory
comment.

Yes, that was my conclusion too, but I wanted to leave the decision up
to you and Baji Shaik.

The attached is what I had in mind (including a regression test to
keep from tempting anyone else), with maybe some minor adjustments in
the comments.

--
John Naylor
Amazon Web Services

LGTM.

+	 * We cannot use ssup_datum_unsigned_cmp here, since we cannot count on
+	 * Datums being zero-extended.

One nitpick that this explains the why, but it doesn't mention the difference with oid8.

On Fri, Jul 24, 2026 at 7:04 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

LGTM.

+        * We cannot use ssup_datum_unsigned_cmp here, since we cannot count on
+        * Datums being zero-extended.

One nitpick that this explains the why, but it doesn't mention the
difference with oid8.

Do you mean prefacing the above with "Unlike the oid8 case, ..." ?

--
John Naylor
Amazon Web Services

On Fri, 24 Jul 2026 at 06:34, John Naylor <johncnaylorls@gmail.com> wrote:

On Thu, Jul 23, 2026 at 12:26 PM Zsolt Parragi
<zsolt.parragi@percona.com> wrote:

I think this commit caused a regression with OIDs >= 2**31:

CREATE TABLE t (o oid);
INSERT INTO t VALUES ('2147483648');
SELECT o FROM t UNION ALL SELECT '3000000000'::oid ORDER BY 1;

Thanks for reporting! Here's the problem: An oid from a heap tuple
goes through fetch_att(), which uses Int32GetDatum for all 4-byte
byval types, which sign-extends. An oid produced by oidin() goes
through ObjectIdGetDatum, which zero-extends. That didn't matter for
btoidfastcmp(), since it compared via DatumGetObjectId(x).

I think the easiest fix is to revert the oid part of commit 51cd5d6f0,
leaving behind the int2 and oid8 parts. The asymmetry between the 2
oid types would look odd, though, so that would require an explanatory
comment.

I didn't see it mentioned, but just for the archives' sake, did you
rule out adding a dedicated uint32 comparator function?

Or is there some other reason this can't be done due to the radix sort code?

David

On Tue, Aug 4, 2026 at 7:09 PM David Rowley <dgrowleyml@gmail.com> wrote:

On Fri, 24 Jul 2026 at 06:34, John Naylor <johncnaylorls@gmail.com> wrote:

I think the easiest fix is to revert the oid part of commit 51cd5d6f0,
leaving behind the int2 and oid8 parts. The asymmetry between the 2
oid types would look odd, though, so that would require an explanatory
comment.

I didn't see it mentioned, but just for the archives' sake, did you
rule out adding a dedicated uint32 comparator function?

Or is there some other reason this can't be done due to the radix sort code?

To be honest, I hadn't put much thought into it, but it seems like a
good invariant to keep that all integer types with normal comparison
semantics are eligible for radix sort. v2 goes in this direction, and
I've run the same tests used when developing radix sort.

On Fri, Jul 24, 2026 at 7:04 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

+        * We cannot use ssup_datum_unsigned_cmp here, since we cannot count on
+        * Datums being zero-extended.

One nitpick that this explains the why, but it doesn't mention the
difference with oid8.

How about:

+ /*
+ * We cannot use ssup_datum_unsigned_cmp here, since the upper half of a
+ * Datum containing a 32-bit type is not reliably zero-extended.
+ */

...by mentioning 32-bit the difference from oid8 should be obvious, I hope.

--
John Naylor
Amazon Web Services

On Thu, 6 Aug 2026 at 19:55, John Naylor <johncnaylorls@gmail.com> wrote:

On Tue, Aug 4, 2026 at 7:09 PM David Rowley <dgrowleyml@gmail.com> wrote:

On Fri, 24 Jul 2026 at 06:34, John Naylor <johncnaylorls@gmail.com> wrote:

I think the easiest fix is to revert the oid part of commit 51cd5d6f0,
leaving behind the int2 and oid8 parts. The asymmetry between the 2
oid types would look odd, though, so that would require an explanatory
comment.

I didn't see it mentioned, but just for the archives' sake, did you
rule out adding a dedicated uint32 comparator function?

Or is there some other reason this can't be done due to the radix sort code?

To be honest, I hadn't put much thought into it, but it seems like a
good invariant to keep that all integer types with normal comparison
semantics are eligible for radix sort. v2 goes in this direction, and
I've run the same tests used when developing radix sort.

The patch looks how I thought it would.

On Fri, Jul 24, 2026 at 7:04 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

+        * We cannot use ssup_datum_unsigned_cmp here, since we cannot count on
+        * Datums being zero-extended.

One nitpick that this explains the why, but it doesn't mention the
difference with oid8.

How about:

+ /*
+ * We cannot use ssup_datum_unsigned_cmp here, since the upper half of a
+ * Datum containing a 32-bit type is not reliably zero-extended.
+ */

...by mentioning 32-bit the difference from oid8 should be obvious, I hope.

Is it ever necessary to have that as a comment? Maybe it'd be better
to rename ssup_datum_unsigned_cmp to ssup_datum_uint64_cmp. It just
doesn't seem questionable why you'd use the 64-bit version for a
32-bit type with those names.

David

On Thu, Aug 6, 2026 at 3:29 PM David Rowley <dgrowleyml@gmail.com> wrote:

On Fri, Jul 24, 2026 at 7:04 PM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

+ /*
+ * We cannot use ssup_datum_unsigned_cmp here, since the upper half of a
+ * Datum containing a 32-bit type is not reliably zero-extended.
+ */

...by mentioning 32-bit the difference from oid8 should be obvious, I hope.

Is it ever necessary to have that as a comment? Maybe it'd be better
to rename ssup_datum_unsigned_cmp to ssup_datum_uint64_cmp. It just
doesn't seem questionable why you'd use the 64-bit version for a
32-bit type with those names.

Yeah, self-documenting code is best. I'll plan on pushing both the
rename and v2 early next week. Thanks for looking!

--
John Naylor
Amazon Web Services

On Thu, Aug 6, 2026 at 5:46 PM John Naylor <johncnaylorls@gmail.com> wrote:

Yeah, self-documenting code is best. I'll plan on pushing both the
rename and v2 early next week. Thanks for looking!

This is done.

--
John Naylor
Amazon Web Services