Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Stephen Frost]

Andres,

* Andres Freund (and...@anarazel.de) wrote:
> I see a new warning due to, presumably, this:
> /home/andres/src/postgresql/src/backend/utils/adt/mac8.c: In function 
> ‘hex2_to_uchar’:
> /home/andres/src/postgresql/src/backend/utils/adt/mac8.c:71:23: warning: 
> comparison is always false due to limited range of data type [-Wtype-limits]
>   if (*ptr < 0 || *ptr > 127)

Pushed a fix for this (which also does a bit of other tidying too).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Stephen Frost]

Andres,

* Andres Freund (and...@anarazel.de) wrote:
> I see a new warning due to, presumably, this:
> /home/andres/src/postgresql/src/backend/utils/adt/mac8.c: In function 
> ‘hex2_to_uchar’:
> /home/andres/src/postgresql/src/backend/utils/adt/mac8.c:71:23: warning: 
> comparison is always false due to limited range of data type [-Wtype-limits]
>   if (*ptr < 0 || *ptr > 127)
>^

Ah, yeah, I suppose I can drop that half of the check.

Will push a fix soon.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Andres Freund]

On 2017-03-15 11:20:53 -0400, Stephen Frost wrote:
> Greetings Hari Babu,
> 
> * Haribabu Kommi (kommi.harib...@gmail.com) wrote:
> > On Mon, Mar 13, 2017 at 6:52 AM, Stephen Frost  wrote:
> > > And, naturally, re-reading the email as it hit the list made me realize
> > > that the documentation/error-message incorrectly said "3rd and 4th"
> > > bytes were being set to FF and FE, when it's actually the 4th and 5th
> > > byte.  The code was correct, just the documentation and the error
> > > message had the wrong numbers.  The commit message is also correct.
> > 
> > Thanks for the review and corrections.
> > 
> > I found some small corrections.
> > 
> > s/4rd/4th/g -- Type correction.
> > 
> > + Input is accepted in the following formats:
> > 
> > As we are supporting many different input variants, and all combinations
> > are not listed, so how about changing the above statement as below.
> > 
> > "Following are the some of the input formats that are accepted:"
> 
> Good points, I incorporated them along with a bit of additional
> information in the documentation as to what we do actually support.
> 
> > I didn't find any other problems during testing and review. The patch
> > is fine.
> 
> Great!  I've committed this now.  If you see anything additional or
> other changes which should be made, please let me know.
> 
> ... and bumped catversion after (thanks for the reminder, Robert!).

I see a new warning due to, presumably, this:
/home/andres/src/postgresql/src/backend/utils/adt/mac8.c: In function 
‘hex2_to_uchar’:
/home/andres/src/postgresql/src/backend/utils/adt/mac8.c:71:23: warning: 
comparison is always false due to limited range of data type [-Wtype-limits]
  if (*ptr < 0 || *ptr > 127)
   ^
Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Haribabu Kommi]

On Thu, Mar 16, 2017 at 2:20 AM, Stephen Frost  wrote:

> Greetings Hari Babu,
>
> * Haribabu Kommi (kommi.harib...@gmail.com) wrote:
> > On Mon, Mar 13, 2017 at 6:52 AM, Stephen Frost 
> wrote:
> > > And, naturally, re-reading the email as it hit the list made me realize
> > > that the documentation/error-message incorrectly said "3rd and 4th"
> > > bytes were being set to FF and FE, when it's actually the 4th and 5th
> > > byte.  The code was correct, just the documentation and the error
> > > message had the wrong numbers.  The commit message is also correct.
> >
> > Thanks for the review and corrections.
> >
> > I found some small corrections.
> >
> > s/4rd/4th/g -- Type correction.
> >
> > + Input is accepted in the following formats:
> >
> > As we are supporting many different input variants, and all combinations
> > are not listed, so how about changing the above statement as below.
> >
> > "Following are the some of the input formats that are accepted:"
>
> Good points, I incorporated them along with a bit of additional
> information in the documentation as to what we do actually support.
>
> > I didn't find any other problems during testing and review. The patch
> > is fine.
>
> Great!  I've committed this now.  If you see anything additional or
> other changes which should be made, please let me know.
>
> ... and bumped catversion after (thanks for the reminder, Robert!).
>

Thanks for the review and changes.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Stephen Frost]

Greetings Hari Babu,

* Haribabu Kommi (kommi.harib...@gmail.com) wrote:
> On Mon, Mar 13, 2017 at 6:52 AM, Stephen Frost  wrote:
> > And, naturally, re-reading the email as it hit the list made me realize
> > that the documentation/error-message incorrectly said "3rd and 4th"
> > bytes were being set to FF and FE, when it's actually the 4th and 5th
> > byte.  The code was correct, just the documentation and the error
> > message had the wrong numbers.  The commit message is also correct.
> 
> Thanks for the review and corrections.
> 
> I found some small corrections.
> 
> s/4rd/4th/g -- Type correction.
> 
> + Input is accepted in the following formats:
> 
> As we are supporting many different input variants, and all combinations
> are not listed, so how about changing the above statement as below.
> 
> "Following are the some of the input formats that are accepted:"

Good points, I incorporated them along with a bit of additional
information in the documentation as to what we do actually support.

> I didn't find any other problems during testing and review. The patch
> is fine.

Great!  I've committed this now.  If you see anything additional or
other changes which should be made, please let me know.

... and bumped catversion after (thanks for the reminder, Robert!).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Haribabu Kommi]

On Mon, Mar 13, 2017 at 6:52 AM, Stephen Frost  wrote:

> Greetings,
>
> * Stephen Frost (sfr...@snowman.net) wrote:
> > * Stephen Frost (sfr...@snowman.net) wrote:
> > > * Haribabu Kommi (kommi.harib...@gmail.com) wrote:
> > > > On Wed, Feb 1, 2017 at 6:27 AM, Vitaly Burovoy <
> vitaly.buro...@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.
>
> And, naturally, re-reading the email as it hit the list made me realize
> that the documentation/error-message incorrectly said "3rd and 4th"
> bytes were being set to FF and FE, when it's actually the 4th and 5th
> byte.  The code was correct, just the documentation and the error
> message had the wrong numbers.  The commit message is also correct.
>

Thanks for the review and corrections.

I found some small corrections.

s/4rd/4th/g -- Type correction.

+ Input is accepted in the following formats:

As we are supporting many different input variants, and all combinations
are not listed, so how about changing the above statement as below.

"Following are the some of the input formats that are accepted:"

I didn't find any other problems during testing and review. The patch
is fine.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Haribabu Kommi]

On Mon, Mar 13, 2017 at 6:38 AM, Stephen Frost  wrote:

> Greetings,
>
> * Stephen Frost (sfr...@snowman.net) wrote:
> > * Haribabu Kommi (kommi.harib...@gmail.com) wrote:
> > > On Wed, Feb 1, 2017 at 6:27 AM, Vitaly Burovoy <
> vitaly.buro...@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.
>

Thanks for the review.


> 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?
>

No. There is no special reason to disallow.
The rewrite of macaddr8_in will allow all possible combinations of spacers.


> 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.
>

I checked and it is working as expected.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Stephen Frost]

Greetings,

* Stephen Frost (sfr...@snowman.net) wrote:
> * Stephen Frost (sfr...@snowman.net) wrote:
> > * Haribabu Kommi (kommi.harib...@gmail.com) wrote:
> > > On Wed, Feb 1, 2017 at 6:27 AM, Vitaly Burovoy  
> > > 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.

And, naturally, re-reading the email as it hit the list made me realize
that the documentation/error-message incorrectly said "3rd and 4th"
bytes were being set to FF and FE, when it's actually the 4th and 5th
byte.  The code was correct, just the documentation and the error
message had the wrong numbers.  The commit message is also correct.

As a reminder to myself, this will also need a catversion bump when it
gets committed, of course.

Updated patch attached.

Thanks!

Stephen
From 5b470010cacdc32bec521b1d57ad9f60c7bd638a Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 9 Mar 2017 17:47:28 -0500
Subject: [PATCH] Add support for EUI-64 MAC addresses as macaddr8

This adds in support for EUI-64 MAC addresses by adding a new data type
called 'macaddr8' (using our usual convention of indicating the number
of bytes stored).

This was largely a copy-and-paste from the macaddr data type, with
appropriate adjustments for having 8 bytes instead of 6 and adding
support for converting a provided EUI-48 (6 byte format) to the EUI-64
format.  Conversion from EUI-48 to EUI-64 inserts FFFE as the 4th and
5th bytes but does not perform the IPv6 modified EUI-64 action of
flipping the 7th bit, but we add a function to perform that specific
action for the user as it may be commonly done by users who wish to
calculate their IPv6 address based on their network prefix and 48-bit
MAC address.

Author: Haribabu Kommi, with a good bit of rework of macaddr8_in by me.
Reviewed by: Vitaly Burovoy, Kuntal Ghosh

Discussion: https://postgr.es/m/CAJrrPGcUi8ZH+KkK+=tctnq+efkecehtmu_yo1mvx8hsk_g...@mail.gmail.com
---
 contrib/btree_gin/Makefile  |   4 +-
 contrib/btree_gin/btree_gin--1.0--1.1.sql   |  35 ++
 contrib/btree_gin/btree_gin.c   |  10 +
 contrib/btree_gin/btree_gin.control |   2 +-
 contrib/btree_gin/expected/macaddr8.out |  51 +++
 contrib/btree_gin/sql/macaddr8.sql  |  22 ++
 contrib/btree_gist/Makefile |  11 +-
 contrib/btree_gist/btree_gist--1.3--1.4.sql |  64 
 contrib/btree_gist/btree_gist.control   |   2 +-
 contrib/btree_gist/btree_gist.h |   1 +
 contrib/btree_gist/btree_macaddr8.c | 200 ++
 contrib/btree_gist/expected/macaddr8.out|  89 +
 contrib/btree_gist/sql/macaddr8.sql |  37 ++
 doc/src/sgml/brin.sgml  |  11 +
 doc/src/sgml/btree-gin.sgml |   3 +-
 doc/src/sgml/btree-gist.sgml|   4 +-
 doc/src/sgml/datatype.sgml  |  83 +
 doc/src/sgml/func.sgml  |  56 +++
 src/backend/utils/adt/Makefile  |   2 +-
 src/backend/utils/adt/mac.c |  13 +-
 src/backend/utils/adt/mac8.c| 560 
 src/backend/utils/adt/network.c |  10 +
 src/backend/utils/adt/selfuncs.c|   1 +
 src/include/catalog/pg_amop.h   |  18 +
 src/include/catalog/pg_amproc.h |   7 +
 src/include/catalog/pg_cast.h   |   6 +
 src/include/catalog/pg_opclass.h|   3 +
 src/include/catalog/pg_operator.h   |  23 +-
 src/include/catalog/pg_opfamily.h   |   3 +
 src/include/catalog/pg_proc.h   |  37 +-
 src/include/catalog/pg_type.h   |   4 +
 src/include/utils/inet.h|  22 ++
 src/test/regress/expected/macaddr8.out  | 355 ++
 src/test/regress/expected/opr_sanity.out|   6 +
 src/test/regress/parallel_schedule  |   2 +-
 src/test/regress/serial_schedule|   1 +
 src/test/regress/sql/macaddr8.sql   |  89 +
 37 files changed, 1827 insertions(+), 20 deletions(-)
 create mode 100644 contrib/btree_gin/btree_gin--1.0--1.1.sql
 create mode 100644 contrib/btree_gin/expected/macaddr8.out
 create mode 100644 contrib/btree_gin/sql/macaddr8.sql
 create mode 100644 contrib/btree_gist/btree_gist--1.3--1.4.sql
 create mode 100644 contrib/btree_gist/btree_macaddr8.c
 create mode 100644 contrib/btree_gist/expected/macaddr8.out
 create mode 100644 contrib/btree_gist/sql/macaddr8.sql
 create mode 100644 src/backend/utils/adt/mac8.c
 create mode 100644 src/test/regress/expected/macaddr8.out
 create mode 100644 src/test/regress/sql/macaddr8.sql

diff --git 

Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Stephen Frost]

Greetings,

* Stephen Frost (sfr...@snowman.net) wrote:
> * Haribabu Kommi (kommi.harib...@gmail.com) wrote:
> > On Wed, Feb 1, 2017 at 6:27 AM, Vitaly Burovoy  
> > 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
From 239affc0e5b09964029f075c5370556c56de005d Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 9 Mar 2017 17:47:28 -0500
Subject: [PATCH] Add support for EUI-64 MAC addresses as macaddr8

This adds in support for EUI-64 MAC addresses by adding a new data type
called 'macaddr8' (using our usual convention of indicating the number
of bytes stored).

This was largely a copy-and-paste from the macaddr data type, with
appropriate adjustments for having 8 bytes instead of 6 and adding
support for converting a provided EUI-48 (6 byte format) to the EUI-64
format.  Conversion from EUI-48 to EUI-64 adds FFFE as the 4th and 5th
bytes but does not perform the IPv6 modified EUI-64 action of flipping
the 7th bit, but we add a function to perform that specific action for
the user as it may be commonly done by users who wish to calculate their
IPv6 address based on their network prefix and 48-bit MAC address.

Author: Haribabu Kommi, with a good bit of rework of macaddr8_in by me.
Reviewed by: Vitaly Burovoy, Kuntal Ghosh

Discussion: https://postgr.es/m/CAJrrPGcUi8ZH+KkK+=tctnq+efkecehtmu_yo1mvx8hsk_g...@mail.gmail.com
---
 contrib/btree_gin/Makefile  |   4 +-
 contrib/btree_gin/btree_gin--1.0--1.1.sql   |  35 ++
 contrib/btree_gin/btree_gin.c   |  10 +
 contrib/btree_gin/btree_gin.control |   2 +-
 contrib/btree_gin/expected/macaddr8.out |  51 +++
 contrib/btree_gin/sql/macaddr8.sql  |  22 ++
 contrib/btree_gist/Makefile |  11 +-
 contrib/btree_gist/btree_gist--1.3--1.4.sql |  64 
 contrib/btree_gist/btree_gist.control   |   2 +-
 

Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Stephen Frost]

Hari Babu,

* Haribabu Kommi (kommi.harib...@gmail.com) wrote:
> On Wed, Feb 1, 2017 at 6:27 AM, Vitaly Burovoy  
> 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.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Haribabu Kommi]

On Wed, Feb 1, 2017 at 6:27 AM, Vitaly Burovoy 
wrote:

> Hello,
>
> I've reviewed the patch[1].
>
> Result of testing:
>
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   tested, passed
> Documentation:tested, passed
>
>
> The patch introduce a new type macaddr8 for EUI-64 addresses[2]
> (assuming OUI field is 24 bits wide) with EUI-48 (existing "macaddr"
> type) interoperability.
> It is a mostly copy-pasted macaddr implementation with necessary
> changes for increased range.
> Consensus was reached on such implementation in the current thread before.
>
> There are two patch files for convenient reviewing: base macaddr8
> implementation and its supporting in btree-gin and btree-gist indexes.
>
> The patch:
> * cleanly applies to the current master
> (6af8b89adba16f97bee0d3b01256861e10d0e4f1);
> * passes tests;
> * looks fine, follows the PostgreSQL style guide;
> * has documentation changes;
> * has tests.
>
> All notes and requirements were took into account and the patch was
> changed according to them.
> I have no suggestions on improving it.
>
> The new status of this patch is: Ready for Committer
>

Thanks for the review.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Kuntal Ghosh]

On Wed, Feb 1, 2017 at 12:57 AM, Vitaly Burovoy
 wrote:
> Hello,
>
> I've reviewed the patch[1].
>
Noting to add from my side as well.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support

[Vitaly Burovoy]

Hello,

I've reviewed the patch[1].

Result of testing:

make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed


The patch introduce a new type macaddr8 for EUI-64 addresses[2]
(assuming OUI field is 24 bits wide) with EUI-48 (existing "macaddr"
type) interoperability.
It is a mostly copy-pasted macaddr implementation with necessary
changes for increased range.
Consensus was reached on such implementation in the current thread before.

There are two patch files for convenient reviewing: base macaddr8
implementation and its supporting in btree-gin and btree-gist indexes.

The patch:
* cleanly applies to the current master
(6af8b89adba16f97bee0d3b01256861e10d0e4f1);
* passes tests;
* looks fine, follows the PostgreSQL style guide;
* has documentation changes;
* has tests.

All notes and requirements were took into account and the patch was
changed according to them.
I have no suggestions on improving it.

The new status of this patch is: Ready for Committer

P.S.:
1. The doc and error/hint messages should be proof-read by a native speaker.
2. A committer should bump catversion. It is not bumped in the patch
because it is unclear when it is committed.

[1]https://postgr.es/m/CAJrrPGeT8zrGPMcRVk_wRvYD-ETcgUz6WRrc2C=inubmrkr...@mail.gmail.com
[2]http://standards.ieee.org/develop/regauth/tut/eui64.pdf
-- 
Best regards,
Vitaly Burovoy


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers