Обсуждение: [PATCH] Simplify SortSupport implementation for macaddr

Hi,

Since Datums are 64-bit values and MAC addresses have only 6 bytes,
the abbreviated key contains the entire MAC address and is
authoritative, as pointed out by John Naylor [1]

This fact eliminates the need for cardinality estimation using
HyperLogLog since macaddr_abbrev_convert() is dirt cheap and not
lossy. There are no reasons to give up on abbreviation.

Potentially we could go even further and pass MAC addresses by value
rather by reference [2]. This would eliminate the need of abbreviation
completely since SortSupport->comparator could just compare two
Datums, as we do for Timestamps. This is a more invasive change though
that deserves more discussion and thus not proposed here.

[1]: https://postgr.es/m/CANWCAZYWdOEnoL_88VpMge1RtRpBz-VRCjdcu-eA4q3U6LvpDw%40mail.gmail.com
[2]: https://postgre.es/m/CAJ7c6TM8up%3DYih8pRLPy4wHwLzHf7w22tQ80-8ZBm__E%3D8_5HA%40mail.gmail.com

-- 
Best regards,
Aleksander Alekseev

> On 25 Feb 2026, at 18:05, Aleksander Alekseev <aleksander@tigerdata.com> wrote:
>
> <v1-0001-Simplify-SortSupport-implementation-for-macaddr.patch>

The patch looks correct and useful to me.

Two small points:

1. The assignment ssup->ssup_extra = NULL can be removed. The
   SortSupport struct is zeroed before the callback is called (see
   sortsupport.h). There are about 22 similar assignments elsewhere;
   it does not seem to be established practice, many other places have
   no such assignment.

2. I checked whether the existing tests actually use the SortSupport
   path. If macaddr_abbrev_abort() is made to error out, the macaddr.sql
   tests fail in two places: once for the index and once for the SELECT.
   So the current test file does exercise the SortSupport code path. I
   also tried making macaddr_abbrev_abort() always return true (so
   abbreviation is always aborted); the test dataset is small, but
   sorting seem to produce correct results.

The patch looks good to me.


Best regards, Andrey Borodin.

Hi Andrey,

Many thanks for your feedback!

> 1. The assignment ssup->ssup_extra = NULL can be removed. The
>    SortSupport struct is zeroed before the callback is called (see
>    sortsupport.h). There are about 22 similar assignments elsewhere;
>    it does not seem to be established practice, many other places have
>    no such assignment.

Agree. I removed this assignment in 0001 and added 0002 that removes
if for the rest of *_sortsupport() functions.

-- 
Best regards,
Aleksander Alekseev

On Mon, Mar 09, 2026 at 04:08:06PM +0300, Aleksander Alekseev wrote:
>> 1. The assignment ssup->ssup_extra = NULL can be removed. The
>>    SortSupport struct is zeroed before the callback is called (see
>>    sortsupport.h). There are about 22 similar assignments elsewhere;
>>    it does not seem to be established practice, many other places have
>>    no such assignment.
>
> Agree. I removed this assignment in 0001 and added 0002 that removes
> if for the rest of *_sortsupport() functions.

Sounds sensible to get rid of the estimation with the Datum size
requirement and never give up with the abbreviated key sort, as done
in 0001.  I'd leave the code touched by 0002 remain as-is.

@Tom, why didn't you consider that as a continuation of 6aebedc38497?
Just to keep the changes with SIZEOF_DATUM < 8 leaner, or just because
it was not worth bothering based on your TODO list?  I am wondering if
there is a reason I may be missing here.
--
Michael