Re: [PATCH] SortSupport for macaddr type

On Wed, Sep 14, 2016 at 6:14 AM, Julien Rouhaud
<julien(dot)rouhaud(at)dalibo(dot)com> wrote:
> 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?

Since it's been two weeks and this patch hasn't been updated in
response to this review, I have marked it "Returned with Feedback" in
the CommitFest. If it is updated, it can be resubmitted for the next
CommitFest.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company