Re: [PATCH] SortSupport for macaddr type

On 26/08/2016 19:44, Brandur wrote:
> Hello,
>

Hello,

> I've attached a patch to add SortSupport for Postgres' macaddr which has the
> effect of improving the performance of sorting operations for the type. The
> strategy that I employ is very similar to that for UUID, which is to create
> abbreviated keys by packing as many bytes from the MAC address as possible into
> Datums, and then performing fast unsigned integer comparisons while sorting.
>
> I ran some informal local benchmarks, and for cardinality greater than 100k
> rows, I see a speed up on `CREATE INDEX` of roughly 25% to 65%. (For those
> interested, I put a few more numbers into a small report here [2].)
>

That's a nice improvement!

> Admittedly, this is not quite as useful as speeding up sorting on a more common
> data type like TEXT or UUID, but the change still seems like a useful
> performance improvement. I largely wrote it as an exercise to familiarize
> myself with the Postgres codebase.
>
> I'll add an entry into the current commitfest as suggested by the Postgres Wiki
> and follow up here with a link.
>
> Thanks, and if anyone has feedback or other thoughts, let me know!
>

I just reviewed your patch. It applies and compiles cleanly, and the
abbrev feature works as intended. There's not much to say since this is
heavily inspired on the uuid SortSupport. The only really specific part
is in the abbrev_converter function, and I don't see any issue with it.

I have a few trivial comments:

* you used macaddr_cmp_internal() function name, for uuid the same
function is named uuid_internal_cmp(). Using the same naming pattern is
probably better.

* the function comment on macaddr_abbrev_convert() doesn't mention
specific little-endian handling

* "There will be two bytes of zero padding on the least significant end"

"least significant bits" would be better

* This patch will trigger quite a lot modifications after a pgindent
run. Could you try to run pgindent on mac.c before sending an updated
patch?

Best regards.

--
Julien Rouhaud
http://dalibo.com - http://dalibo.org