Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support
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
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
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
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
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
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
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
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 a/contrib/btree_gin/Makefile b/contrib/btree_gin/Makefile index 0492091.
Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support
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 +- contrib/btree_gist/btree_gist.h | 1 + contrib/btree
Re: [HACKERS][REVIEW] macaddr 64 bit (EUI-64) datatype support
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
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
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