On Wed, Sep 14, 2016 at 6:14 AM, Julien Rouhaud <julien.rouh...@dalibo.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers