Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support - Mailing list pgsql-hackers

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Haribabu Kommi (kommi.haribabu@gmail.com) wrote:
> > On Wed, Feb 1, 2017 at 6:27 AM, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:
> > > The new status of this patch is: Ready for Committer
> >
> > Thanks for the review.
>
> I've started taking a look at this with an eye towards committing it
> soon.

I've spent a good bit of time going over this, possibly even more than
it was worth, but hopefully we'll see people making use of this data
type with PG10 and as more IPv6 deployment happens.

Of particular note, I rewrote macaddr8_in to not use sscanf().
sscanf(), at least on my system, would accept negative values even for
'%2x', leading to slightly odd errors with certain inputs, including
with our existing macaddr type:

=# select '00-203040506'::macaddr;
ERROR:  invalid octet value in "macaddr" value: "00-203040506"
LINE 1: select '00-203040506'::macaddr;

With my rewrite, the macaddr8 type will throw a clearer (in  my view, at
least) error:

=# select '00-203040506'::macaddr8;
ERROR:  invalid input syntax for type macaddr8: "00-203040506"
LINE 1: select '00-203040506'::macaddr8;

One other point is that the previously disallowed format with just two
colons ('0800:2b01:0203') is now allowed.  Given that both the two dot
format ('0800.2b01.0203') and the two dash format ('0800-2b01-0203')
were accepted, this seemed alright to me.  Is there really a good reason
to disallow the two colon format?

I didn't change how macaddr works as it doesn't appear to produce any
outright incorrect behavior as-is (just slightly odd error messages) and
some users might be expecting the current errors.  I don't hold that
position very strongly, however, and I have little doubt that the new
macaddr8_in() is faster than using sscanf(), so that might be reason to
consider rewriting macaddr_in in a similar fashion (or having a generic
set of functions to handle both).  I considered using the functions we
already use for bytea, but they don't quite match up to the expectations
for MAC addresses (in particular, we shouldn't accept random whitespace
in the middle of a MAC address).  Perhaps we could modify those
functions to be parameterized in a way to support how a MAC address
should look, but it's not all that much code to be reason enough to put
a lot of effort towards that, in my view at least.  This also reduces
the risk that bugs get introduced which break existing behavior too.

I also thought about what we expect the usage of macaddr8 to be and
realized that we should really have a function to help go from EUI-48 to
the IPv6 Modified EUI-64 format, since users will almost certainly want
to do exactly that.  As such, I added a macaddr8_set7bit() function
which will perform the EUI-64 -> Modified EUI-64 change (which is just
setting the 7th bit) and added associated documentation and reasoning
for why that function exists.

In any case, it would be great to get some additional review of this, in
particular of my modifications to macaddr8_in() and if anyone has any
thoughts regarding the added macaddr8_set7bit() function.  I'm going to
take a break from it for a couple days to see if there's any additional
comments and then go over it again myself.

Barring issues, I'll commit the attached later this week.

Thanks!

Stephen