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

On Thu, 6 Aug 2026 at 19:55, John Naylor <johncnaylorls(at)gmail(dot)com> wrote:
>
> On Tue, Aug 4, 2026 at 7:09 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> >
> > On Fri, 24 Jul 2026 at 06:34, John Naylor <johncnaylorls(at)gmail(dot)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(dot)parragi(at)percona(dot)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