On Tue, Aug 4, 2026 at 7:09 PM David Rowley <[email protected]> wrote: > > On Fri, 24 Jul 2026 at 06:34, John Naylor <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> Reviewed-by: Zsolt Parragi <[email protected]> Suggested-by: David Rowley <[email protected]> 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_tuple(state->memtuples, @@ -3471,6 +3474,20 @@ ssup_datum_signed_cmp(Datum x, Datum y, SortSupport ssup) return 0; } +int +ssup_datum_uint32_cmp(Datum x, Datum y, SortSupport ssup) +{ + uint32 xx = DatumGetUInt32(x); + uint32 yy = DatumGetUInt32(y); + + if (xx < yy) + return -1; + else if (xx > yy) + return 1; + else + return 0; +} + int ssup_datum_int32_cmp(Datum x, Datum y, SortSupport ssup) { diff --git a/src/include/utils/sortsupport.h b/src/include/utils/sortsupport.h index a8f8f9f026a..41923f6364c 100644 --- a/src/include/utils/sortsupport.h +++ b/src/include/utils/sortsupport.h @@ -274,6 +274,7 @@ ApplySortAbbrevFullComparator(Datum datum1, bool isNull1, */ extern int ssup_datum_unsigned_cmp(Datum x, Datum y, SortSupport ssup); extern int ssup_datum_signed_cmp(Datum x, Datum y, SortSupport ssup); +extern int ssup_datum_uint32_cmp(Datum x, Datum y, SortSupport ssup); extern int ssup_datum_int32_cmp(Datum x, Datum y, SortSupport ssup); /* Other functions in utils/sort/sortsupport.c */ 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 '3000000000'::oid ORDER BY 1; + o +------------ + 2147483647 + 2147483648 + 3000000000 +(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 '3000000000'::oid ORDER BY 1; -- 2.55.0
