Re: [PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support
On Thu, Aug 6, 2026 at 3:29 PM David Rowley wrote: > > On Fri, Jul 24, 2026 at 7:04 PM Zsolt Parragi > > 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
Re: [PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support
On Thu, 6 Aug 2026 at 19:55, John Naylor wrote: > > On Tue, Aug 4, 2026 at 7:09 PM David Rowley wrote: > > > > On Fri, 24 Jul 2026 at 06:34, John Naylor 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 > 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
Re: [PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support
On Tue, Aug 4, 2026 at 7:09 PM David Rowley wrote:
>
> On Fri, 24 Jul 2026 at 06:34, John Naylor 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 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
From 8d4be376b89995a7c4ddbe872b79f7f0739e98e3 Mon Sep 17 00:00:00 2001
From: John Naylor
Date: Thu, 23 Jul 2026 16:19:57 -0400
Subject: [PATCH v2] Add ssup_datum_uint32_cmp for comparing oids
Commit 51cd5d6f0 used ssup_datum_unsigned_cmp for the oid comparator,
which compares entire Datums. That gave wrong results, since the upper
half of a Datum containing a 32-bit type is not reliably zero-extended:
values fetched from tuples are sign-extended, while values returned
by e.g. oidin() are zero-extended. An oid with the high bit set could
therefore compare as larger or smaller depending on where it came from.
Fix by adding a comparator that only looks at the low 32 bits. Also
teach radix sort to normalize Datums the same way. This keeps oid
eligible for radix sort. Add a regression test that sorts oids above
2^31 from both kinds of source.
Reported-by: Zsolt Parragi
Reviewed-by: Zsolt Parragi
Suggested-by: David Rowley
Discussion: https://postgr.es/m/can4czfm2afrzsljjiwknrjqpwl7scm2wd1xxu140dijgujt...@mail.gmail.com
---
src/backend/access/nbtree/nbtcompare.c | 6 +++-
src/backend/utils/sort/tuplesort.c | 39 ++---
src/include/utils/sortsupport.h | 1 +
src/test/regress/expected/tuplesort.out | 11 +++
src/test/regress/sql/tuplesort.sql | 5
5 files changed, 50 insertions(+), 12 deletions(-)
diff --git a/src/backend/access/nbtree/nbtcompare.c b/src/backend/access/nbtree/nbtcompare.c
index 795fded49d3..7edfa737d7a 100644
--- a/src/backend/access/nbtree/nbtcompare.c
+++ b/src/backend/access/nbtree/nbtcompare.c
@@ -427,7 +427,11 @@ btoidsortsupport(PG_FUNCTION_ARGS)
{
SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);
- ssup->comparator = ssup_datum_unsigned_cmp;
+ /*
+ * 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.
+ */
+ ssup->comparator = ssup_datum_uint32_cmp;
PG_RETURN_VOID();
}
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index c0e7527b9ca..ee9c89b02d3 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -2588,24 +2588,26 @@ normalize_datum(Datum orig, SortSupport ssup)
Datum norm_datum1;
if (ssup->comparator == ssup_datum_signed_cmp)
- {
norm_datum1 = orig + (Int64GetDatum(PG_INT64_MAX)) + 1;
- }
- else if (ssup->comparator == ssup_datum_int32_cmp)
+ else if (ssup->comparator == ssup_datum_unsigned_cmp)
+ norm_datum1 = orig;
+ else
{
/*
- * First truncate to uint32. Technically, we don't need to do this,
+ * Truncate to uint32. For the int32 case, we don't need to do this,
* but it forces the upper half of the datum to be zero regardless of
* sign.
*/
- uint32 u32 = DatumGetUInt32(orig) + ((uint32) PG_INT32_MAX) + 1;
+ uint32 u32 = DatumGetUInt32(orig);
+
+ if (ssup->comparator == ssup_datum_int32_cmp)
+ norm_datum1 = UInt32GetDatum(u32 + ((uint32) PG_INT32_MAX) + 1);
+ else
+ {
+ norm_datum1 = UInt32GetDatum(u32);
+ Assert(ssup->comparator == ssup_datum_uint32_cmp);
+ }
- norm_datum1 = UInt32GetDatum(u32);
- }
- else
- {
- Assert(ssup->comparator == ssup_datum_unsigned_cmp);
- norm_datum1 = orig;
}
if (ssup->ssup_reverse)
@@ -3011,6 +3013,7 @@ tuplesort_sort_memtuples(Tuplesortstate *state)
if (state->memtupcount >= QSORT_THRESHOLD &&
(ssup->comparator == ssup_datum_unsigned_cmp ||
ssup->comparator == ssup_datum_signed_cmp ||
+ ssup->comparator == ssup_datum_uint32_cmp ||
ssup->comparator == ssup_datum_int32_cmp))
{
radix_sort_tu
Re: [PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support
On Fri, 24 Jul 2026 at 06:34, John Naylor wrote:
>
> On Thu, Jul 23, 2026 at 12:26 PM Zsolt Parragi
> 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 '30'::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
Re: [PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support
On Fri, Jul 24, 2026 at 7:04 PM Zsolt Parragi 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
Re: [PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support
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.
Re: [PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support
On Thu, Jul 23, 2026 at 3:15 PM Zsolt Parragi 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
From f8a61f9c1ac1850d67823cdefab9368199e84d33 Mon Sep 17 00:00:00 2001
From: John Naylor
Date: Thu, 23 Jul 2026 16:19:57 -0400
Subject: [PATCH v1] Revert using ssup_datum_unsigned_cmp for comparing oids
---
src/backend/access/nbtree/nbtcompare.c | 20 +++-
src/test/regress/expected/tuplesort.out | 11 +++
src/test/regress/sql/tuplesort.sql | 5 +
3 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/nbtree/nbtcompare.c b/src/backend/access/nbtree/nbtcompare.c
index 795fded49d3..a797912ff81 100644
--- a/src/backend/access/nbtree/nbtcompare.c
+++ b/src/backend/access/nbtree/nbtcompare.c
@@ -422,12 +422,30 @@ btoidcmp(PG_FUNCTION_ARGS)
PG_RETURN_INT32(A_LESS_THAN_B);
}
+static int
+btoidfastcmp(Datum x, Datum y, SortSupport ssup)
+{
+ Oid a = DatumGetObjectId(x);
+ Oid b = DatumGetObjectId(y);
+
+ if (a > b)
+ return A_GREATER_THAN_B;
+ else if (a == b)
+ return 0;
+ else
+ return A_LESS_THAN_B;
+}
+
Datum
btoidsortsupport(PG_FUNCTION_ARGS)
{
SortSupport ssup = (SortSupport) PG_GETARG_POINTER(0);
- ssup->comparator = ssup_datum_unsigned_cmp;
+ /*
+ * We cannot use ssup_datum_unsigned_cmp here, since we cannot count on
+ * Datums being zero-extended.
+ */
+ ssup->comparator = btoidfastcmp;
PG_RETURN_VOID();
}
diff --git a/src/test/regress/expected/tuplesort.out b/src/test/regress/expected/tuplesort.out
index fc1321bf443..9851e8a6a57 100644
--- a/src/test/regress/expected/tuplesort.out
+++ b/src/test/regress/expected/tuplesort.out
@@ -703,3 +703,14 @@ EXPLAIN (COSTS OFF) :qry;
(10 rows)
COMMIT;
+-- Test sorting oids with the high bit set, mixing sources
+CREATE TEMP TABLE test_oid_sort (o oid);
+INSERT INTO test_oid_sort VALUES ('2147483648'), ('2147483647');
+SELECT o FROM test_oid_sort UNION ALL SELECT '30'::oid ORDER BY 1;
+ o
+
+ 2147483647
+ 2147483648
+ 30
+(3 rows)
+
diff --git a/src/test/regress/sql/tuplesort.sql b/src/test/regress/sql/tuplesort.sql
index 8476e594e6c..31cd28c6116 100644
--- a/src/test/regress/sql/tuplesort.sql
+++ b/src/test/regress/sql/tuplesort.sql
@@ -305,3 +305,8 @@ EXPLAIN (COSTS OFF) :qry;
:qry;
COMMIT;
+
+-- Test sorting oids with the high bit set, mixing sources
+CREATE TEMP TABLE test_oid_sort (o oid);
+INSERT INTO test_oid_sort VALUES ('2147483648'), ('2147483647');
+SELECT o FROM test_oid_sort UNION ALL SELECT '30'::oid ORDER BY 1;
--
2.55.0
Re: [PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support
> 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.
Re: [PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support
On Thu, Jul 23, 2026 at 12:26 PM Zsolt Parragi
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 '30'::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
Re: [PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support
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 '30'::oid ORDER BY 1;
Re: [PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support
On Thu, Jun 4, 2026 at 6:37 AM Baji Shaik 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
Re: [PATCH] Use ssup_datum_*_cmp for int2, oid, and oid8 sort support
On Thu, Jun 4, 2026 at 10:45 PM Michael Paquier 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/
Re: [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]? > 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 signature.asc Description: PGP signature
