Re: [PATCH] SortSupport for macaddr type

From: Julien Rouhaud <julien(dot)rouhaud(at)dalibo(dot)com>
To: Brandur Leach <brandur(at)mutelight(dot)org>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] SortSupport for macaddr type
Date: 2017-02-25 17:56:20
Message-ID: 20170225175620.GA7238@nol.local
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Sun, Feb 05, 2017 at 01:56:41PM -0800, Brandur Leach wrote:

Hello Brandur, thanks for the updated patch!

>
> > * 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.
>
> I was a little split on this one! It's true that UUID uses
> `_internal_cmp`, but `_cmp_internal` is also used in a
> number of places like `enum`, `timetz`, and `network`. I
> don't have a strong feeling about it either way, so I've
> changed it to `_internal_cmp` to match UUID as you
> suggested.
>

Indeed, I should have checked more examples :/ There isn't any clear pattern
for this, so I guess any one would be ok.

> > * the function comment on macaddr_abbrev_convert()
> > doesn't mention specific little-endian handling
>
> I tried to bake this into the comment text. Here are the
> relevant lines of the amended version:
>
> * Packs the bytes of a 6-byte MAC address into a Datum and treats it as
> an
> * unsigned integer for purposes of comparison. On a 64-bit machine,
> there
> * will be two zeroed bytes of padding. The integer is converted to
> native
> * endianness to facilitate easy comparison.
>
> > * "There will be two bytes of zero padding on the least
> > significant end"
> >
> > "least significant bits" would be better
>
> Also done. Here is the amended version:
>
> * On a 64-bit machine, zero out the 8-byte datum and copy the 6 bytes of
> * the MAC address in. There will be two bytes of zero padding on the end
> * of the least significant bits.
>

Thanks. I'm ok with this, but maybe a native english speaker would have a
better opinion on this.

> > * 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?
>
> Good call! I've run the new version through pgindent.
>

Thanks also, no more issue here.

> Let me know if you have any further feedback and/or
> suggestions. Thanks!

One last thing, I noticed that you added:

+static int macaddr_internal_cmp(macaddr *a1, macaddr *a2);

but the existing function is declared as

static int32
macaddr_internal_cmp(macaddr *a1, macaddr *a2)

I'd be in favor to declare both as int.

After this, I think this patch will be ready for committer.

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Greg Stark 2017-02-25 18:11:23 Re: UPDATE of partition key
Previous Message Simon Riggs 2017-02-25 17:54:24 Reduce lock levels for reloptions