Thread: macaddr 64 bit (EUI-64) datatype support

On Wed, Oct 12, 2016 at 3:30 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> As we are moving to PostgreSQL 10, so are there any plans of backward
> compatiblity
> breakage, so the existing macaddr datatype itself can be changed to support
> both
> 48 and 64 bit MAC addresses.

Er, I had thought that we should not use Postgres 10 as a reason for
backward-incompatible breakages, at least not to increase them into a
such amount that upgrading would be a pain.
-- 
Michael

On 12 October 2016 at 14:30, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:

> As we are moving to PostgreSQL 10, so are there any plans of backward
> compatiblity
> breakage, so the existing macaddr datatype itself can be changed to support
> both
> 48 and 64 bit MAC addresses. If not, I will try update the POC patch with
> more details
> similar like macaddr datatype.

There's been some minor BC breaking, but breaking on-disk format for
pg_upgrade is a much bigger deal. I'm really not a fan of that idea.

Just use macaddr64 if you want wide MACs.


-- Craig Ringer                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On 12/10/2016 09:32, Craig Ringer wrote:
> On 12 October 2016 at 14:30, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> 
>> As we are moving to PostgreSQL 10, so are there any plans of backward
>> compatiblity
>> breakage, so the existing macaddr datatype itself can be changed to support
>> both
>> 48 and 64 bit MAC addresses. If not, I will try update the POC patch with
>> more details
>> similar like macaddr datatype.
> 
> There's been some minor BC breaking, but breaking on-disk format for
> pg_upgrade is a much bigger deal. I'm really not a fan of that idea.
> 
> Just use macaddr64 if you want wide MACs.
> 
> 

+1

and you can instead make macaddr64 support both format, and provide a
macaddr::macaddr64 cast

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

Julien Rouhaud wrote:

> and you can instead make macaddr64 support both format, and provide a
> macaddr::macaddr64 cast

Having macaddr64 support both formats sounds nice, but how does it work?
Will we have to reserve one additional bit to select the representation?
That would make the type be 65 bits which is a clear loser IMO.

Is it allowed to just leave 16 bits as zeroes which would indicate that
the address is EUI48?  I wouldn't think so ...

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> Here I attached a POC patch that adds the support for EUI-64 MAC address
> storage with a new datatype macaddr64. Currently this type takes only
> EUI-64 datatype, not accepts 48 bit MAC address.

Our other data types that have sizes in the names measure the sizes in
bytes (float4, int8, etc).  Should this be called macaddr8?

> As we are moving to PostgreSQL 10, so are there any plans of backward
> compatiblity breakage, so the existing macaddr datatype itself can be
> changed to support both 48 and 64 bit MAC addresses.

As others have noted, there is no likelihood that we'd take a disk-format-
compatibility-breaking patch for v10.  Even if we wanted to do that, the
above proposal would also break send/recv (binary COPY) compatibility for
macaddr.

I think that probably the best bet here is to have two types and put some
thought into making them interoperate where appropriate, as the various
sizes of int do.  It's kind of a shame that this won't look like the
approach used for inet addresses, but we're stuck.
        regards, tom lane

On 12/10/2016 14:32, Alvaro Herrera wrote:
> Julien Rouhaud wrote:
> 
>> and you can instead make macaddr64 support both format, and provide a
>> macaddr::macaddr64 cast
> 
> Having macaddr64 support both formats sounds nice, but how does it work?
> Will we have to reserve one additional bit to select the representation?
> That would make the type be 65 bits which is a clear loser IMO.
> 
> Is it allowed to just leave 16 bits as zeroes which would indicate that
> the address is EUI48?  I wouldn't think so ...
> 

From what I read, you can indicate it's an EUI-48 address by storing
FF:FF (or FF:FE for MAC-48) in 4th and 5th bytes of the EUI-64 address.

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

Julien Rouhaud wrote:
> On 12/10/2016 14:32, Alvaro Herrera wrote:
> > Julien Rouhaud wrote:
> > 
> >> and you can instead make macaddr64 support both format, and provide a
> >> macaddr::macaddr64 cast
> > 
> > Having macaddr64 support both formats sounds nice, but how does it work?
> > Will we have to reserve one additional bit to select the representation?
> > That would make the type be 65 bits which is a clear loser IMO.
> > 
> > Is it allowed to just leave 16 bits as zeroes which would indicate that
> > the address is EUI48?  I wouldn't think so ...
> 
> From what I read, you can indicate it's an EUI-48 address by storing
> FF:FF (or FF:FE for MAC-48) in 4th and 5th bytes of the EUI-64 address.

That seems reasonable at first glance; so the new type would be able to
store both 48-bit and 64-bit values, and there would be assignment casts
in both directions and a suite of operators to enable interoperability
of 48-bit values in macaddr8 with values in type macaddr.  Right?

(The cast function from macaddr8 to macaddr would raise error if the
4th and 5th bytes are not either FF:FF or FF:FE -- I don't think we can
in practice distinguish EUI-48 from MAC-48 in this context.  The cast in
the other direction would have no restriction and should probably always
use FF:FE).

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 10/12/16, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Julien Rouhaud wrote:
>> On 12/10/2016 14:32, Alvaro Herrera wrote:
>> > Julien Rouhaud wrote:
>> >
>> >> and you can instead make macaddr64 support both format, and provide a
>> >> macaddr::macaddr64 cast
>> >
>> > Having macaddr64 support both formats sounds nice, but how does it
>> > work?
>> > Will we have to reserve one additional bit to select the
>> > representation?
>> > That would make the type be 65 bits which is a clear loser IMO.
>> >
>> > Is it allowed to just leave 16 bits as zeroes which would indicate that
>> > the address is EUI48?  I wouldn't think so ...
>>
>> From what I read, you can indicate it's an EUI-48 address by storing
>> FF:FF (or FF:FE for MAC-48) in 4th and 5th bytes of the EUI-64 address.
>
> That seems reasonable at first glance; so the new type would be able to
> store both 48-bit and 64-bit values, and there would be assignment casts
> in both directions

I think either "macaddr" should be renamed to "macaddr6" (saved its
oid), a new type "macaddr8" is added with introducing a new alias
"macaddr" or the current "macaddr" should accept both versions as the
"inet" type does.

The function "macaddr_recv" can distinguish them by the
StringInfoData.len member, "macaddr_in" - by number of parts split by
":".
The "macaddr_out" and "macaddr_send" can out 6 octets if the stored
value is mapped to MAC-48.
Storing can be done always as 8 bytes using the rule above.

In the other case there is hard from user's PoV to detect at the
design stage when it is necessary to define column as macaddr and when
to macaddr8.
If users need to update a column type (a new hardware appears with
EUI-64 address), they should keep in mind that all things are changed
for the new column type - stored procedure's parameters, application
code interacting with the DB etc.).
I don't agree with Tom's proposal to introduce a new type, it would be
inconvenient for users.

We have types "int2", "int4", "int8" and an alias "int" for a type
"int4", at least psql does not show it:
postgres=# \dT+ int                                   List of data typesSchema | Name | Internal name | Size | Elements
|Owner | Access
 
privileges | Description
--------+------+---------------+------+----------+-------+-------------------+-------------
(0 rows)

It is understandable to have 3 types for integers because most space
of the DB occupied by them and in the real life we just don't use big
numbers, but cases for "inet" and "macaddr" are different.

> and a suite of operators to enable interoperability
> of 48-bit values in macaddr8 with values in type macaddr.  Right?
>
> (The cast function from macaddr8 to macaddr would raise error if the
> 4th and 5th bytes are not either FF:FF or FF:FE -- I don't think we can
> in practice distinguish EUI-48 from MAC-48 in this context.

The wikipedia says[1] they are the same things but MAC-48 is an
obsolete term for a special case, so there is no necessary to
distinguish them.

> The cast in the other direction would have no restriction and should
> probably always use FF:FE).

[1]
https://en.wikipedia.org/wiki/Organizationally_unique_identifier#48-bit_Media_Access_Control_Identifier_.28MAC-48.29
-- 
Best regards,
Vitaly Burovoy

On 10/12/16, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:
> On 10/12/16, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>> Julien Rouhaud wrote:
>>> On 12/10/2016 14:32, Alvaro Herrera wrote:
>>> > Julien Rouhaud wrote:
>>> >
>>> >> and you can instead make macaddr64 support both format, and provide a
>>> >> macaddr::macaddr64 cast
>>> >
>>> > Having macaddr64 support both formats sounds nice, but how does it
>>> > work?
>>> > Will we have to reserve one additional bit to select the
>>> > representation?
>>> > That would make the type be 65 bits which is a clear loser IMO.
>>> >
>>> > Is it allowed to just leave 16 bits as zeroes which would indicate
>>> > that
>>> > the address is EUI48?  I wouldn't think so ...
>>>
>>> From what I read, you can indicate it's an EUI-48 address by storing
>>> FF:FF (or FF:FE for MAC-48) in 4th and 5th bytes of the EUI-64 address.
>>
>> That seems reasonable at first glance; so the new type would be able to
>> store both 48-bit and 64-bit values, and there would be assignment casts
>> in both directions
>
> I think either "macaddr" should be renamed to "macaddr6" (saved its
> oid), a new type "macaddr8" is added with introducing a new alias
> "macaddr" or the current "macaddr" should accept both versions as the
> "inet" type does.
>
> The function "macaddr_recv" can distinguish them by the
> StringInfoData.len member, "macaddr_in" - by number of parts split by
> ":".
> The "macaddr_out" and "macaddr_send" can out 6 octets if the stored
> value is mapped to MAC-48.
> Storing can be done always as 8 bytes using the rule above.
>
> In the other case there is hard from user's PoV to detect at the
> design stage when it is necessary to define column as macaddr and when
> to macaddr8.
> If users need to update a column type (a new hardware appears with
> EUI-64 address), they should keep in mind that all things are changed
> for the new column type - stored procedure's parameters, application
> code interacting with the DB etc.).
> I don't agree with Tom's proposal to introduce a new type, it would be
> inconvenient for users.
>
> We have types "int2", "int4", "int8" and an alias "int" for a type
> "int4", at least psql does not show it:
> postgres=# \dT+ int
>                                     List of data types
>  Schema | Name | Internal name | Size | Elements | Owner | Access
> privileges | Description
> --------+------+---------------+------+----------+-------+-------------------+-------------
> (0 rows)
>
> It is understandable to have 3 types for integers because most space
> of the DB occupied by them and in the real life we just don't use big
> numbers, but cases for "inet" and "macaddr" are different.
>
>> and a suite of operators to enable interoperability
>> of 48-bit values in macaddr8 with values in type macaddr.  Right?
>>
>> (The cast function from macaddr8 to macaddr would raise error if the
>> 4th and 5th bytes are not either FF:FF or FF:FE -- I don't think we can
>> in practice distinguish EUI-48 from MAC-48 in this context.
>
> The wikipedia says[1] they are the same things but MAC-48 is an
> obsolete term for a special case, so there is no necessary to
> distinguish them.
>
>> The cast in the other direction would have no restriction and should
>> probably always use FF:FE).
>
> [1]
> https://en.wikipedia.org/wiki/Organizationally_unique_identifier#48-bit_Media_Access_Control_Identifier_.28MAC-48.29

Haribabu Kommi, why have you read enough about EUI-64?
Your function "macaddr64_trunc" sets 4 lower bytes as 0 whereas
(according to the Wikipedia, but I can look for a standard) 3 bytes
are still define an OUI (organizationally unique identifier), so
truncation should be done for 5, not 4 lower octets.

The macros "hibits" should be 3 octets long, not 4; "lobits" --- 5 bytes, not 4.
In the other case your comparisons fail.

What document have you used to write the patch? Are short form formats
correct in macaddr64_in?

P.S.: I still think it is a good idea to change storage format,
macaddr_{in,out,send,recv}, fill 4th and 5th bytes if necessary;
change "lobits" macros and add new fields to bit operation functions.
It avoids a new type, casting, comparison functions between macaddr6
and macaddr8 etc. Proposed patch is mostly copy-paste of mac.c

-- 
Best regards,
Vitaly Burovoy

I'm sorry for the offtopic, but does anyone know a reason why a
condition in mac.c

> if ((a < 0) || (a > 255) || (b < 0) || (b > 255) ||
>     (c < 0) || (c > 255) || (d < 0) || (d > 255) ||
>     (e < 0) || (e > 255) || (f < 0) || (f > 255))

can not be rewritten as:

> if (((a | b | c | d | e | f) < 0) ||
>     ((a | b | c | d | e | f) > 255))

It seems more compact and a compiler can optimize it to keep a result
of a binary OR for the comparison with 255...

-- 
Best regards,
Vitaly Burovoy

Vitaly Burovoy <vitaly.burovoy@gmail.com> writes:
> P.S.: I still think it is a good idea to change storage format,

I'm not sure which part of "no" you didn't understand, but we're
not breaking on-disk compatibility of existing macaddr columns.
Breaking the on-the-wire binary I/O representation seems like a
nonstarter as well.

If you can think of a way to have one type do it all without breaking
that, then fine; but that seems like a pretty hard problem.
        regards, tom lane

Tom Lane wrote:
> Vitaly Burovoy <vitaly.burovoy@gmail.com> writes:
> > P.S.: I still think it is a good idea to change storage format,
> 
> I'm not sure which part of "no" you didn't understand, but we're
> not breaking on-disk compatibility of existing macaddr columns.
> Breaking the on-the-wire binary I/O representation seems like a
> nonstarter as well.

I think the suggestion was to rename macaddr to macaddr6 or similar,
keeping the existing behavior and the current OID.  So existing columns
would continue to work fine and maintain on-disk compatibility, but any
newly created columns would become the 8-byte variant.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Vitaly Burovoy <vitaly.burovoy@gmail.com> writes:
> I'm sorry for the offtopic, but does anyone know a reason why a
> condition in mac.c

>> if ((a < 0) || (a > 255) || (b < 0) || (b > 255) ||
>> (c < 0) || (c > 255) || (d < 0) || (d > 255) ||
>> (e < 0) || (e > 255) || (f < 0) || (f > 255))

> can not be rewritten as:

>> if (((a | b | c | d | e | f) < 0) ||
>> ((a | b | c | d | e | f) > 255))

Well, it's ugly and it adds a bunch of assumptions about arithmetic
behavior that we don't particularly need to make.  If this were some
amazingly hot hot-spot then maybe it would be worth making the code
unreadable to save a few nanoseconds, but I doubt that it is.
(Anyway, you've not shown that there actually is any benefit ...)
        regards, tom lane

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> Vitaly Burovoy <vitaly.burovoy@gmail.com> writes:
>>> P.S.: I still think it is a good idea to change storage format,

>> I'm not sure which part of "no" you didn't understand, but we're
>> not breaking on-disk compatibility of existing macaddr columns.
>> Breaking the on-the-wire binary I/O representation seems like a
>> nonstarter as well.

> I think the suggestion was to rename macaddr to macaddr6 or similar,
> keeping the existing behavior and the current OID.  So existing columns
> would continue to work fine and maintain on-disk compatibility, but any
> newly created columns would become the 8-byte variant.

... and would have different I/O behavior from before, possibly breaking
applications that expect "macaddr" to mean what it used to.  I'm still
dubious that that's a good idea.

The larger picture here is that we got very little thanks when we squeezed
IPv6 into the pre-existing inet datatype; there's a large number of people
who just said "no thanks" and started using the add-on ip4r type instead.
So I'm not sure why we want to complicate our lives in order to make
macaddr follow the same path.
        regards, tom lane

On 10/12/16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Vitaly Burovoy <vitaly.burovoy@gmail.com> writes:
>> I'm sorry for the offtopic, but does anyone know a reason why a
>> condition in mac.c
>
>>> if ((a < 0) || (a > 255) || (b < 0) || (b > 255) ||
>>>     (c < 0) || (c > 255) || (d < 0) || (d > 255) ||
>>>     (e < 0) || (e > 255) || (f < 0) || (f > 255))
>
>> can not be rewritten as:
>
>>> if (((a | b | c | d | e | f) < 0) ||
>>>     ((a | b | c | d | e | f) > 255))
>

> Well, it's ugly and

> it adds a bunch of assumptions about arithmetic
> behavior that we don't particularly need to make.

It explains the reason, thank you. I'm just not familiar with other
architectures where it is not the same as in X86/X86-64.

> If this were some
> amazingly hot hot-spot then maybe it would be worth making the code
> unreadable to save a few nanoseconds, but I doubt that it is.

> (Anyway, you've not shown that there actually is any benefit ...)
I don't think it has a speed benefit, especially comparing with the sscanf call.
But personally for me the second variant does not seem ugly, just "no
one bit in all variables is out of a byte" (looks better with
comparison with "0xff" as sscanf operates with "%2x").
Sorry for my bad taste and for a noise.

-- 
Best regards,
Vitaly Burovoy

On 10/12/16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> Tom Lane wrote:
>>> Vitaly Burovoy <vitaly.burovoy@gmail.com> writes:
>>>> P.S.: I still think it is a good idea to change storage format,

>>> I'm not sure which part of "no" you didn't understand,

I just paid attention to the words "likelihood" (mixed up with
"likeliness"), "we wanted" and "probably".
Also there was a note about "would also break send/recv" which
behavior can be saved.
And after your letter Julien Rouhaud wrote about mapping from MAC-48
to EUI-64 which leads absence of a bit indicated version of a stored
value. Which can be considered as a new information.

>>> but we're
>>> not breaking on-disk compatibility of existing macaddr columns.

Can I ask why? It will not be a varlen (typstorage will not be
changed), it just changes typlen to 8 and typalign to 'd'.
For every major release 9.0, 9.1, 9.2 .. 9.6 the docs says "A
dump/restore using pg_dumpall, or use of pg_upgrade, is required".
Both handle changes in a storage format. Do they?

>>> Breaking the on-the-wire binary I/O representation seems like a
>>> nonstarter as well.

I wrote that for the EUI-48 (MAC-48) values the binary I/O
representation can be saved.
The binary format (in DataRow message) has a length of the column
value which is reflected in PGresAttValue.len in libpq.
If the client works with the binary format it must consult with the
length of the data.
But until the client works with (and columns have) MAC-48 nothing
hurts it and PGresAttValue.len is "6" as now.

>> I think the suggestion was to rename macaddr to macaddr6 or similar,
>> keeping the existing behavior and the current OID.  So existing columns
>> would continue to work fine and maintain on-disk compatibility, but any
>> newly created columns would become the 8-byte variant.
>
> ... and would have different I/O behavior from before, possibly breaking
> applications that expect "macaddr" to mean what it used to.  I'm still
> dubious that that's a good idea.

Only if a new type will send xx:xx:xx:FF:FF:xx:xx:xx instead of usual
(expected) 6 octets long.
Again, that case in my offer is similar (by I/O behavior) to "just
change 'macaddr' to keep and accept both MAC-48 and MAC-64", but
allows to use "-k" key for pg_upgrade to prevent rewriting possibly
huge (for instance, 'log') tables (but users unexpectedly get
"macaddr6" after upgrade in their columns and function names which
looks strange enough).

> The larger picture here is that we got very little thanks when we squeezed
> IPv6 into the pre-existing inet datatype;

Without a sarcasm, I thank a lot all people involved in it because it
does not hurt me (and many other people) from distinguishing ipv4 and
ipv6 at app-level.
I write apps and just save remote address of clients to an "inet"
column named "remote_ip" without thinking "what if we start serving
clients via ipv6?"; or have a column named "allowed_ip" with IPs or
subnets and just save client's IPv4 or IPv6 as a white list (and use
"allowed_ip >>= $1"). It just works.

> there's a large number of people
> who just said "no thanks" and started using the add-on ip4r type instead.

I found a repository[1] at github. From the description it is
understandable why people used ip4r those days (2005 year). The reason
"Furthermore, they are variable length types (to support ipv6) with
non-trivial overheads" is mentioned as the last in its README.
When you deal with IPv4 in 99.999%, storing it in TOAST tables leads
to a big penalty, but the new version of macaddr is not so wide, so it
does not lead to similar speed decrease (it will be stored inplace).

> So I'm not sure why we want to complicate our lives in order to make
> macaddr follow the same path.

Because according to the Wiki[3] MAC addresses now "are formed
according to the rules of one of three numbering name spaces ...:
MAC-48, EUI-48, and EUI-64.", so IEEE extended range of allowed values
from 48 to 64 bits and since Postgres claims supporting of "mac
addresses", I (as a developer who still uses PG as a primary database)
expect supporting of any kind of mac address, not a limited one. I
expect it is just works.

I reject to imagine what I have to do if I have a column of a type
"macaddr" and unexpectedly I have to deal with an input of EUI-64
type. Add a new column or change columns's type?

In the first case what to do with stored procedures? Duplicate input
parameter to pass the new column of macaddr8 (if macaddr was passed
before)? Duplicate stored procedure?
Also I have to support two columns at the application level. Why? I
just want to store it in the DB, work with it there and get it back!

In the second case (if output will not be mapped to MAC-48 when it is
possible) I have the same troubles as you wrote (oid, I/O and text
representation at least for output, may be also for input).
Moreover I still have to rewrite tables but not when I'm ready for it
(at a migration stage from one major version to another), but when the
task appears.

===
I see no type (besides integers, floats and related with them: their
ranges and arrays ) where numbers appears indicating their capacity:

postgres=# select typname from pg_type where typname ~ '[0-9]' and
typname not like 'pg_toast_%';  typname

-------------int8int2int2vectorint4float4float8_int2_int2vector_int4_int8_float4_float8int4range_int4rangeint8range_int8range
(16 rows)

So why should we have the name "macaddr" without capacity number and
(unexpectedly) macaddr8 (when a different number appears in the
official name "EAI-64")?

===
I offer a change when the current behavior is not changed for MAC-48
values at all (for textual and binary I/O), internal representation is
always 64bit long, and input and output are mapped from (and when it
is possible to) MAC-48 to seamless usage of a "mac address" concept.


P.S.: Note that the current version[2] of ip4r has the "ipaddress"
type for both IPv4 and IPv6 like the "inet" has. We'll end up having a
single type for both MAC-48 and MAC-64. Why don't do it immediately
(without intermediate types)?
While time passes more and more hardware have EUI-64; the same as more
and more clients have IPv6.

P.P.S.: I played around a length of a value in the binary format (in a
client and in the "macaddr_recv"). It is possible to distinguish
MAC-48 to EUI-64 inputs in "macaddr_recv", so there is no changes
necessary at the client side while it works with the MAC-48 format
only.


[1] https://github.com/petere/ip4r-cvs
[2] https://github.com/RhodiumToad/ip4r
[3] https://en.wikipedia.org/wiki/MAC_address

-- 
Best regards,
Vitaly Burovoy

On 10/12/16 4:59 PM, Tom Lane wrote:
> The larger picture here is that we got very little thanks when we squeezed
> IPv6 into the pre-existing inet datatype; there's a large number of people
> who just said "no thanks" and started using the add-on ip4r type instead.

I don't think that is a correct account.  People used the ip4r extension
because it was faster, had more functionality, and didn't have those
stupid network masks to worry about.  ip4r does in fact also provide a
type that can contain ip4 and ip6, which one ought to use nowadays.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Oct 12, 2016 at 08:33:00AM -0400, Tom Lane wrote:
> As others have noted, there is no likelihood that we'd take a disk-format-
> compatibility-breaking patch for v10.  Even if we wanted to do that, the
> above proposal would also break send/recv (binary COPY) compatibility for
> macaddr.
> 
> I think that probably the best bet here is to have two types and put some
> thought into making them interoperate where appropriate, as the various
> sizes of int do.  It's kind of a shame that this won't look like the
> approach used for inet addresses, but we're stuck.

If feels like we are going into VARCHAR2 territory where we end up
telling people to use an oddly-named data type forever.  Some are
suggesting JSONB is in that category.

I wish I had a suggestion, but I am not above adding trickery to
pg_upgrade to improve the outcome.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On 10/25/16 1:38 AM, Haribabu Kommi wrote:
> Here I attached the first version of patch that supports both EUI-48 and
> EUI-64 type
> Mac addresses with a single datatype called macaddr. This is an variable
> length
> datatype similar like inet. It can store both 6 and 8 byte addresses.
> Variable length
> type is used because in case in future, if MAC address gets enhanced,
> still this type
> can support without breaking DISK compatibility.

Since the world knows the 6-byte variant as MAC-48, shouldn't it be
renamed to macaddr48 or even mac48?

> Currently the patch lacks of documentation. Comments?

For patches like this, it would be good if you included a mock commit
message so that someone who comes in late knows what's going on.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Oct 18, 2016 at 7:25 PM, Bruce Momjian <bruce@momjian.us> wrote:
> On Wed, Oct 12, 2016 at 08:33:00AM -0400, Tom Lane wrote:
>> As others have noted, there is no likelihood that we'd take a disk-format-
>> compatibility-breaking patch for v10.  Even if we wanted to do that, the
>> above proposal would also break send/recv (binary COPY) compatibility for
>> macaddr.
>>
>> I think that probably the best bet here is to have two types and put some
>> thought into making them interoperate where appropriate, as the various
>> sizes of int do.  It's kind of a shame that this won't look like the
>> approach used for inet addresses, but we're stuck.
>
> If feels like we are going into VARCHAR2 territory where we end up
> telling people to use an oddly-named data type forever.  Some are
> suggesting JSONB is in that category.

I think that's just the price of maintaining a stable database system
over the course of many years.  You end up with a few warts in the
name of backward compatibility.  It's not wonderful to have warts, but
backward compatibility has enough value to make it worth enduring the
warts.  If the worst thing that happens is we end up with types called
jsonb and macaddr8, we're doing well.  Obviously, there's some point
at which maintaining backward compatibility becomes too much of a
nuisance to be worthwhile, and that's why we've periodically made
compatibility breaks of various types.  I'm sure we'll continue to do
that in the future from time to time, but I wouldn't do it here.  I
think Tom's got the right idea: let's leave the existing datatype
alone to avoid breaking things for people who are already using it,
and add a new one for people to enable the new functionality.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11/4/16 4:55 AM, Shay Rojansky wrote:
>     1. Does everyone agrees that renaming the existing datatype without
>     changing the OID?
> 
> 
> As I said before, Npgsql for one loads data types by name, not by OID.
> So this would definitely cause breakage.

Why would that cause breakage?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

* Shay Rojansky (roji@roji.org) wrote:
> >
> > > As I said before, Npgsql for one loads data types by name, not by OID.
> >>> > So this would definitely cause breakage.
> >>>
> >>> Why would that cause breakage?
> >>
> >> Well, the first thing Npgsql does when it connects to a new database, is
> >> to query pg_type. The type names are used to associate entries with type
> >> handlers, avoiding the hard-coding of OIDs in code. So if the type name
> >> "macaddr" suddenly has a new meaning and its wire representation is
> >> different breakage will occur. It is possible to release new versions of
> >> Npgsql which will look at the PostgreSQL version and say "we know that in
> >> PostgreSQL < 10 macaddr means this, but in >= 10 it means that". But that
> >> doesn't seem like a good solution, plus old versions of Npgsql from before
> >> this change won't work.
> >
> > The new datatype that is going to replace the existing one works with both
> > 6 and 8 byte
> > MAC address and stores it a variable length format. This doesn't change
> > the wire format.
> > I don't see any problem with the existing applications. The new datatype
> > can recv and send
> > 8 byte MAC address also.
>
> Apologies, I may have misunderstood. If the new type is 100%
> wire-compatible (recv/send) with the old fixed-length 6-byte type, then
> there's no issue whatsoever.

Uh, that certainly isn't correct, is it...?

The new data type is going to be able to send back 8-byte values to a
Npgsql driver that's not expecting to get back 8 byte macaddrs.  That
isn't going to work.

Further, I don't agree that we can simply rename the existing macaddr to
something else, even if we keep the same OID, that's going to confuse
the heck out of people who are doing pg_upgrade's 9.6 to 10 and then
they try to do migrations using their previous systems or even to
compare the output of pg_dump from one to the next.

Let's create a new data type for this which supports old and new types,
add what casts make sense, and call it a day.  Changing the data type
name out from under people is not helping anyone.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Let's create a new data type for this which supports old and new types,
> add what casts make sense, and call it a day.  Changing the data type
> name out from under people is not helping anyone.

+1.  I do not think changing behavior for the existing type name is
going to be a net win.  If we'd been smart enough to make the type
varlena from the get-go, maybe we could get away with it, but there
is just way too much risk of trouble with a change in a fixed-length
type's on-the-wire representation.
        regards, tom lane

On Sat, Nov 19, 2016 at 2:54 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Stephen Frost <sfrost@snowman.net> writes:
>> Let's create a new data type for this which supports old and new types,
>> add what casts make sense, and call it a day.  Changing the data type
>> name out from under people is not helping anyone.
>
> +1.  I do not think changing behavior for the existing type name is
> going to be a net win.  If we'd been smart enough to make the type
> varlena from the get-go, maybe we could get away with it, but there
> is just way too much risk of trouble with a change in a fixed-length
> type's on-the-wire representation.

I completely agree.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> Any suggestions for the name to be used for the new datatype the can
> work for both 48 and 64 bit MAC addresses?

The precedent of int4/int8/float4/float8 is that SQL data types should
be named after their length in bytes.  So I'd be inclined to call this
"macaddr8" not "macaddr64".  That would suggest taking the simple
approach of always storing values in the 8-byte format, rather than
dealing with the complexities of having two formats internally, two
display formats, etc.

> It is possible to represent a 48 bit MAC address as 64 bit MAC address
> by adding reserved bytes in the middle as follows.
> 01-01-01-01-01-01::macaddr => 01-01-01-FF-FE-01-01-01::newmacaddr

Check.  So we could accept 6-byte addresses on input and perform
that conversion automatically.

> While comparing a 48 bit MAC address with 64 bit MAC address, Ignore
> the two bytes if the contents in those bytes are reserved bytes.

Um ... I don't follow.  Surely these must compare different:

01-01-01-FF-FE-01-01-01
01-01-01-FF-0E-01-01-01

> The new datatype can store directly whatever is the input is, like 48 bit
> or 64 bit. The same data is sent over the wire, whether the reserved
> bytes are present or not?

I'd just send all 8 bytes.  What the client wants to do with values
that could be mapped back into the 6-byte space is its business.

In short, let's just make this work similarly to integers of different
widths, rather than trying to sprinkle extra pixie dust on it.
        regards, tom lane

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> On Wed, Nov 23, 2016 at 1:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The precedent of int4/int8/float4/float8 is that SQL data types should
>> be named after their length in bytes.  So I'd be inclined to call this
>> "macaddr8" not "macaddr64".  That would suggest taking the simple
>> approach of always storing values in the 8-byte format, rather than
>> dealing with the complexities of having two formats internally, two
>> display formats, etc.

> Do you prefer the automatic conversion from 6 byte to 8 byte MAC address,
> This way it takes extra space with new datatype. Is it fine to with new
> datatype?

Well, I think the space savings would be pretty illusory.  If we use a
varlena datatype, then old-style MAC addresses would take 7 bytes and
new-style would take 9.  That's not much of an improvement over always
taking 8 bytes.  What's worse, if the next field has an alignment
requirement more than char, is that it's really 8 bytes and 12 bytes (or
16!), making this strictly worse than a fixed-length-8-bytes approach.

As against that, if we use a varlena type then we'd have some protection
against the day when the MAC people realize they were still being
short-sighted and go up to 10 or 12 or 16 bytes.  But even if that happens
while Postgres is still in use, I'm not sure that we'd choose to make use
of the varlena aspect rather than invent a third datatype to go with that
new version of the standard.  Per the discussion in this thread, varlena
storage in itself doesn't do very much for the client-side compatibility
issues.  Making a new datatype with a new, well-defined I/O behavior
ensures that applications don't get blindsided by a new behavior they're
not ready for.

In short, I'm leaning to having just a fixed-length-8-byte implementation.
This may seem like learning nothing from our last go-round, but the
advantages of varlena are very far in the hypothetical future, and the
disadvantages are immediate.

Also, if we define macaddr as "always 6 bytes" and macaddr8 as "always 8
bytes", then there's a very simple way for users to widen an old-style
address to 8 bytes or convert it back to the 6-byte format: just cast
to the other datatype.  If the new macaddr type can store both widths
then you need to invent at least one additional function to provide
those behaviors.
        regards, tom lane

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> Currently the casting is supported from macaddr to macaddr8, but not the
> other way. This is because, not all 8 byte MAC addresses can be converted
> into 6 byte addresses.

Well, yeah, so you'd throw an error if it can't be converted.  This is
no different from casting int8 to int4, for example.  We don't refuse
to provide that cast just because it will fail for some values.
        regards, tom lane

On 1/4/17, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> On Tue, Nov 29, 2016 at 8:36 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>> Updated patch attached with added cast function from macaddr8 to
>> macaddr.
>>
>> Currently there are no support for cross operators. Is this required
>> to be this patch only or can be handled later if required?
>>
>
> Updated patch attached to address the duplicate OID problem.
> There are no other functional changes to the previous patch.

Hello,

several thoughts about the patch:


Documentation:
1.
+     The remaining six input formats are not part of any standard.
Which ones (remaining six formats)?


2.
+ <function>trunc(<type>macaddr8</type>)</function></literal> returns a MAC
+   address with the last 3 bytes set to zero.
It is a misprinting or a copy-paste error.
The implementation and the standard says that the last five bytes are
set to zero and the first three are left as is.


3.
+ for lexicographical ordering
I'm not a native English speaker, but I'd say just "for ordering"
since there are no words, it is just a big number with a special
input/output format.


The code:
4.
+    if ((a == 0) && (b == 0) && (c == 0) && (d == 0)
+            && (e == 0) && (f == 0) && (g == 0) && (h == 0))
...
+    if ((a == 255) && (b == 255) && (c == 255) && (d == 255)
+            && (e == 255) && (f == 255) && (g == 255) && (h == 255))
The standard forbids these values from using in real hardware, no
reason to block them as input data.
Moreover these values can be stored as a result of binary operations,
users can dump them but can not restore.


5.
+    if (!eight_byte_address)
+    {
+        result->d = 0xFF;
...

Don't you think the next version is simplier (all sscanf for macaddr6
skip "d" and "e")?

+    count = sscanf(str, "%x:%x:%x:%x:%x:%x%1s",
+                   &a, &b, &c, &f, &g, &h, junk);
...
+    if (!eight_byte_address)
+    {
+        d = 0xFF;
+        e = 0xFE;
+    }
+    result->a = a;
+    result->b = b;
+    result->c = c;
+    result->d = d;
+    result->e = e;
+    result->f = f;
+    result->g = g;
+    result->h = h;

Also:
+    if (buf->len == 6)
+    {
+        addr->d = 0xFF;
+        addr->e = 0xFE;
+        addr->f = pq_getmsgbyte(buf);
+        addr->g = pq_getmsgbyte(buf);
+        addr->h = pq_getmsgbyte(buf);
+    }
+    else
+    {
+        addr->d = pq_getmsgbyte(buf);
+        addr->e = pq_getmsgbyte(buf);
+        addr->f = pq_getmsgbyte(buf);
+        addr->g = pq_getmsgbyte(buf);
+        addr->h = pq_getmsgbyte(buf);
+    }

can be written as:
+    if (buf->len == 6)
+    {
+        addr->d = 0xFF;
+        addr->e = 0xFE;
+    }
+    else
+    {
+        addr->d = pq_getmsgbyte(buf);
+        addr->e = pq_getmsgbyte(buf);
+    }
+    addr->f = pq_getmsgbyte(buf);
+    addr->g = pq_getmsgbyte(buf);
+    addr->h = pq_getmsgbyte(buf);

but it is only my point of view (don't need to pay close attention
that only those two bytes are written differently, not the last tree
ones), it is not an error.


6.
+                 errmsg("macaddr8 out of range to convert to macaddr")));
I think a hint should be added which values are allowed to convert to "macaddr".

7.
+DATA(insert (  829    774    4123 i f ));
+DATA(insert (  774  829       4124 i f ));
It is a nitpicking, but try to use tabs as in the code around.
(tab between "774" and "829" and space instead of tab between "829" and "4124").

8.
+#define hibits(addr) \
+  ((unsigned long)(((addr)->a<<24)|((addr)->b<<16)|((addr)->c<<8)))
+
+#define lobits(addr) \
+  ((unsigned long)(((addr)->d<<16)|((addr)->e<<8)|((addr)->f)))
+
+#define lobits_extra(addr) \
+  ((unsigned long)((addr)->g<<8)|((addr)->h))

I mentioned that fitting all 4 bytes is a wrong idea[1]
> The macros "hibits" should be 3 octets long, not 4;

... but now I do not think so (there is no UB[2] because source and
destination are not signed).
Moreover you've already fill in "hibits" the topmost byte by shifting by 24.
If you use those two macros ("hibits" and "lobits") it allows to avoid
two extra comparisons in macaddr8_cmp_internal.
Version from the "macaddr64_poc.patch" is correct.


[1]https://www.postgresql.org/message-id/CAKOSWNng9_+=FVO6OZ4TGv1KKHmoM11anKihBoKPuZki1cAkLQ@mail.gmail.com
[2]http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1817.htm


-- 
Best regards,
Vitaly Burovoy

On Mon, Jan 9, 2017 at 1:45 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> Updated patch is attached.
>
I've a few comments about the patch.

+     This type can accept both 6 and 8 bytes length MAC addresses.
A 6 bytes length MAC address is internally converted to 8 bytes. We
should include this in the docs. Because output is always 8 bytes.
Otherwise, a user may be surprised with the output.

+#define hibits(addr) \
+  ((unsigned long)(((addr)->a<<24)|((addr)->b<<16)|((addr)->c<<8)|((addr)->d)))
+
+#define lobits(addr) \
+  ((unsigned long)(((addr)->e<<24)|((addr)->f<<16)|((addr)->g<<8)|((addr)->h)))
+
There should be some spacing.

+ if (!eight_byte_address)
+ {
+ d = 0xFF;
+ e = 0xFE;
+ }
You already have count variable. Just check (count != 8).

+ res *= 256 * 256;
+ res *= 256 * 256;
Bit shift operator can be used for this. For example: (res << 32).

-DATA(insert ( 403 macaddr_ops PGNSP PGUID 1984  829 t 0 ));
-DATA(insert ( 405 macaddr_ops PGNSP PGUID 1985  829 t 0 ));
+DATA(insert ( 403 macaddr_ops PGNSP PGUID 1984  829 t 0 ));
+DATA(insert ( 405 macaddr_ops PGNSP PGUID 1985  829 t 0 ));
This is unnecessary I guess.

There was some whitespace error while applying the patch. Also, there
are some spacing inconsistencies in the comments. I've not tested with
this patch. I'll let you know once I'm done testing.


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

On Thu, Jan 19, 2017 at 7:46 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
>
> Updated patch attached.
>
I've looked at the updated patch. There are still some changes that
needs to be done. It includes:

1. Adding macaddr8 support for btree_gist and testcases.
2. Implementation of macaddr8 support for btree_gin is incomplete. You
need to modify contrib/btree_gin/*.sql files as well. Also, testcases
needs to be added.

Other than that, I've tested the basic implementation of the feature.
It looks good.

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

On 1/23/17, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> The patch is split into two parts.
> 1. Macaddr8 datatype support
> 2. Contrib module support.

Hello,

I'm sorry for the delay.
The patch is almost done, but I have two requests since the last review.

1.
src/backend/utils/adt/mac8.c:
+    int            a,
+                b,
+                c,
+                d = 0,
+                e = 0,
...

There is no reason to set them as 0. For EUI-48 they will be
reassigned in the "if (count != 8)" block, for EUI-64 -- in one of
sscanf.
They could be set to "d = 0xFF, e = 0xFE," and avoid the "if" block
mentioned above, but it makes the code be much less readable.

Oh. I see. In the current version it must be assigned because for
EUI-48 memory can have values <0 or >255 in uninitialized variables.
It is better to avoid initialization by merging two parts of the code:
+    if (count != 6)
+    {
+        /* May be a 8-byte MAC address */
...
+    if (count != 8)
+    {
+        d = 0xFF;
+        e = 0xFE;
+    }

to a single one:
+    if (count == 6)
+    {
+        d = 0xFF;
+        e = 0xFE;
+    }
+    else
+    {
+        /* May be a 8-byte MAC address */
...

2.
src/backend/utils/adt/network.c:
+                res = (mac->a << 24) | (mac->b << 16) | (mac->c << 8) | (mac->d);
+                res = (double)((uint64)res << 32);
+                res += (mac->e << 24) | (mac->f << 16) | (mac->g << 8) | (mac->h);

Khm... I trust that modern compilers can do a lot of optimizations but
for me it looks terrible because of needless conversions.
The reason why earlier versions did have two lines "res *= 256 * 256"
was an integer overflow for four multipliers, but it can be solved by
defining the first multiplier as a double:
+                res = (mac->a << 24) | (mac->b << 16) | (mac->c << 8) | (mac->d);
+                res *= (double)256 * 256 * 256 * 256;
+                res += (mac->e << 24) | (mac->f << 16) | (mac->g << 8) | (mac->h);

In this case the left-hand side argument for the "*=" operator is
computed at the compile time as a single constant.
The second line can be written as "res *= 256. * 256 * 256 * 256;"
(pay attention to a dot in the first multiplier), but it is not
readable at all (and produces the same code).

-- 
Best regards,
Vitaly Burovoy

On 1/25/17, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> On Wed, Jan 25, 2017 at 6:43 PM, Vitaly Burovoy <vitaly.burovoy@gmail.com>
> wrote:
>
>> On 1/23/17, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>> > The patch is split into two parts.
>> > 1. Macaddr8 datatype support
>> > 2. Contrib module support.
>>
>> Hello,
>>
>> I'm sorry for the delay.
>> The patch is almost done, but I have two requests since the last review.
>>
>
> Thanks for the review.
>
>
>> 1.
>> src/backend/utils/adt/mac8.c:
>> +       int                     a,
>> +                               b,
>> +                               c,
>> +                               d = 0,
>> +                               e = 0,
>> ...
>>
>> There is no reason to set them as 0. For EUI-48 they will be
>> reassigned in the "if (count != 8)" block, for EUI-64 -- in one of
>> sscanf.
>> They could be set to "d = 0xFF, e = 0xFE," and avoid the "if" block
>> mentioned above, but it makes the code be much less readable.
>>

<overquoted>

>
> Changed accordingly.

I'm going to do (I hope) a final review tonight.
Please, remove initialization of the variables "d" and "e" since there
is really no reason to keep them be zero for a short time.

-- 
Best regards,
Vitaly Burovoy

On Wed, Jan 25, 2017 at 6:00 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:

> Corrected as suggested.
>
> Updated patch attached. There is no change in the contrib patch.
Got whitspace error warning while applying contrib_macaddr8_1.patch:184.

diff --git a/contrib/btree_gist/btree_gist.control
b/contrib/btree_gist/btree_gist.control
index ddbf83d..fdf0e6a 100644
--- a/contrib/btree_gist/btree_gist.control
+++ b/contrib/btree_gist/btree_gist.control
@@ -1,5 +1,5 @@# btree_gist extensioncomment = 'support for indexing common datatypes in GiST'
-default_version = '1.3'
+default_version = '1.4'
btree_gin.control should be updated as well. Otherwise, make
check-world is throwing error.

After making the above changes, I'm able to run regression test
successfully without any error/warning.

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

On 1/27/17, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> Updated patches are attached.


Hello,

I'm almost ready to mark it as Ready for committer.
The final round.

1.
>+DATA(insert OID = 774 ( macaddr8 ...
>+#define MACADDR8OID 972
What does this number (972) mean? I think it should be 774, the same
number as was used in the type definition.


Since there is an editing required, please, fix whitespaces:
2.
>+DATA(insert OID = 3371 (    403        macaddr8_ops            PGNSP PGUID ));
>+DATA(insert OID = 3372 (    405        macaddr8_ops            PGNSP PGUID ));

only one (not three) tab before "PGNSP" should be used (see other rows
around yours: "OID = 1983", "OID = 1991" etc).

3.
>diff --git a/contrib/btree_gist/Makefile b/contrib/btree_gist/Makefile
some new rows have two tabs instead of eight spaces.

4.
Some other files need to be fixed by pgindent (it helps supporting in
the future):
contrib/btree_gist/btree_macaddr8.c (a lot of rows)
src/include/utils/inet.h  (I have no idea why it changes indentation
to tabs for macaddr8 whereas it leaves spaces for macaddr)

5.
src/backend/utils/adt/network.c
pg_indent makes it uglier. I've just found how to write the line for it:
res *= ((double) 256) * 256 * 256 * 256;

-- 
Best regards,
Vitaly Burovoy

On Tue, Jan 31, 2017 at 2:13 PM, Vitaly Burovoy
<vitaly.burovoy@gmail.com> wrote:
> I'm almost ready to mark it as Ready for committer.
> The final round.

Moved to next CF with same status, waiting on author as the last patch
and the last review are very fresh.
-- 
Michael

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=iNubMRkrMxw@mail.gmail.com
[2]http://standards.ieee.org/develop/regauth/tut/eui64.pdf
-- 
Best regards,
Vitaly Burovoy

On Wed, Feb 1, 2017 at 12:57 AM, Vitaly Burovoy
<vitaly.burovoy@gmail.com> 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

Hari Babu,

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

Thanks!

Stephen

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

Greetings,

* Stephen Frost (sfrost@snowman.net) wrote:
> * 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.

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

Greetings Hari Babu,

* Haribabu Kommi (kommi.haribabu@gmail.com) wrote:
> On Mon, Mar 13, 2017 at 6:52 AM, Stephen Frost <sfrost@snowman.net> 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

On 2017-03-15 11:20:53 -0400, Stephen Frost wrote:
> Greetings Hari Babu,
> 
> * Haribabu Kommi (kommi.haribabu@gmail.com) wrote:
> > On Mon, Mar 13, 2017 at 6:52 AM, Stephen Frost <sfrost@snowman.net> 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
rangeof data type [-Wtype-limits] if (*ptr < 0 || *ptr > 127)                      ^
 
Andres

Andres,

* Andres Freund (andres@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
rangeof 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

Andres,

* Andres Freund (andres@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
rangeof 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