[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]/messages/by-id/CANWCAZYWdOEnoL_88VpMge1RtRpBz-VRCjdcu-eA4q3U6LvpDw@mail.gmail.com
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]https://postgre.es/m/CAJ7c6TM8up%3DYih8pRLPy4wHwLzHf7w22tQ80-8ZBm__E%3D8_5HA%40mail.gmail.com. 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]: /messages/by-id/CANWCAZYWdOEnoL_88VpMge1RtRpBz-VRCjdcu-eA4q3U6LvpDw@mail.gmail.com
[2]: https://postgre.es/m/CAJ7c6TM8up%3DYih8pRLPy4wHwLzHf7w22tQ80-8ZBm__E%3D8_5HA%40mail.gmail.com
--
Best regards,
Aleksander Alekseev
Attachments:
v1-0001-Simplify-SortSupport-implementation-for-macaddr.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Simplify-SortSupport-implementation-for-macaddr.patchDownload+5-96
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
Attachments:
v2-0002-Remove-redundant-ssup_extra-zeroing-in-_sortsuppo.patchtext/x-patch; charset=US-ASCII; name=v2-0002-Remove-redundant-ssup_extra-zeroing-in-_sortsuppo.patchDownload+0-23
v2-0001-Simplify-SortSupport-implementation-for-macaddr.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Simplify-SortSupport-implementation-for-macaddr.patchDownload+5-97
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
On Wed, Mar 25, 2026 at 3:01 PM Michael Paquier <michael@paquier.xyz> wrote:
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.
I've committed v2-0001, thanks Aleksander! Without 0002, 0001 made
this code inconsistent with the tree by removing the assignment, so I
left it as in master.
--
John Naylor
Amazon Web Services