Thread

Commits

Add support for EUI-64 MAC addresses as macaddr8
- c7a9fa399d55 10.0 landed
perltidy pg_dump TAP tests
- 6af8b89adba1 10.0 cited

macaddr 64 bit (EUI-64) datatype support

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-10-12T06:30:52Z

There was bug that is raised in [1] related to storing EUI-64 mac address
in PostgreSQL MAC address datatype.

As the current macaddr datatype stores only 48 bit MAC address only, and
now a days
people are adopting to EUI-64 format of MAC address. So it better to add
the support
in PostgreSQL.

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.

Before continuing and adding more details for macaddr64 datatype, it is not
possible to
add the support for current macaddr datatype as it is a fixed size datatype
that is stored
in the disk. So any enhancements to change it from 48 to 64 bit will give
problems to
pg_upgrade.

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.


[1] -
https://www.postgresql.org/message-id/20161010162001.1413.12486%40wrigleys.postgresql.org

Regards,
Hari Babu
Fujitsu Australia

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

Michael Paquier <michael.paquier@gmail.com> — 2016-10-12T06:52:57Z

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

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

Craig Ringer <craig@2ndquadrant.com> — 2016-10-12T07:32:34Z

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

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

Julien Rouhaud <julien.rouhaud@dalibo.com> — 2016-10-12T08:42:20Z

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

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

Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-10-12T12:32:52Z

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

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

Tom Lane <tgl@sss.pgh.pa.us> — 2016-10-12T12:33:00Z

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

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

Julien Rouhaud <julien.rouhaud@dalibo.com> — 2016-10-12T12:50:51Z

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

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

Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-10-12T16:19:35Z

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

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

Vitaly Burovoy <vitaly.burovoy@gmail.com> — 2016-10-12T17:33:18Z

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
-- 
Best regards,
Vitaly Burovoy

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

Vitaly Burovoy <vitaly.burovoy@gmail.com> — 2016-10-12T20:31:45Z

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

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

Vitaly Burovoy <vitaly.burovoy@gmail.com> — 2016-10-12T20:44:35Z

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

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

Tom Lane <tgl@sss.pgh.pa.us> — 2016-10-12T20:45:54Z

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

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

Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-10-12T20:48:41Z

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

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

Tom Lane <tgl@sss.pgh.pa.us> — 2016-10-12T20:52:07Z

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

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

Tom Lane <tgl@sss.pgh.pa.us> — 2016-10-12T20:59:43Z

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

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

Vitaly Burovoy <vitaly.burovoy@gmail.com> — 2016-10-12T21:11:35Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-10-12T23:44:57Z

On Thu, Oct 13, 2016 at 7:31 AM, Vitaly Burovoy <vitaly.burovoy@gmail.com>
wrote:

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

Yes, OUI is 24 bits. I just created prototype patch to check community
opinion on it.
I checked the following links [1], [2] for the development of macaddr8. But
the patch is
not correct for all the cases, it is just a prototype to see whether it
accepts 8 byte
MAC address or not?

[1] - http://standards.ieee.org/develop/regauth/tut/eui64.pdf
[2] - https://en.wikipedia.org/wiki/MAC_address

Regards,
Hari Babu
Fujitsu Australia

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-10-13T04:12:10Z

On Thu, Oct 13, 2016 at 7:59 AM, 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, 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.
>
>
Thanks for all your opinions regarding the addition of new datatype to
support
EUI-64 Mac address, I will work on it and come up with a patch.


Regards,
Hari Babu
Fujitsu Australia

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

Vitaly Burovoy <vitaly.burovoy@gmail.com> — 2016-10-13T05:10:26Z

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
-------------
 int8
 int2
 int2vector
 int4
 float4
 float8
 _int2
 _int2vector
 _int4
 _int8
 _float4
 _float8
 int4range
 _int4range
 int8range
 _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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-10-17T01:58:11Z

On Thu, Oct 13, 2016 at 4:10 PM, Vitaly Burovoy <vitaly.burovoy@gmail.com>
wrote:

> On 10/12/16, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>> 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?
>

macaddr is not a varlena datatype, all the varlena datatypes stores the
length of the data in the header byte because of variable length data
they can hold.

As the macaddr datatype is not that type, so changing the storage size
from 6 to 8 will break the on-disk compatibility, thus it can cause users
to use only pg_dump to upgrade to version 10. pg_upgrade doesn't
handle the changes in storage format.

Just because of a single datatype, loosing the option of using pg_upgrade
is huge and it is not worth as I feel.

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

By taking some steps, yes, it is possible to accept both 48-bit and 64-bit
address into a single macaddr datatype.

But I feel this should be done with a new datatype and eventually drop
the old datatype after some time.

> ===
> 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
> -------------
>  int8
>  int2
>  int2vector
>  int4
>  float4
>  float8
>  _int2
>  _int2vector
>  _int4
>  _int8
>  _float4
>  _float8
>  int4range
>  _int4range
>  int8range
>  _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.
>

I agree that adding new datatype whenever the standards are changed to
store the MAC address, instead the new datatype that we are going to add
now can changed as an varlena datatype, so it can handle any length mac
addresses.

The current macaddr datatype needs to be kept for some time by renaming
it without changing OID and use the newer one for further usage.

Regards,
Hari Babu
Fujitsu Australia

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

Shay Rojansky <roji@roji.org> — 2016-10-17T14:51:15Z

>
> The current macaddr datatype needs to be kept for some time by renaming
> it without changing OID and use the newer one for further usage.
>

From the point of view of a driver maintainer... Npgsql looks up data types
by their name - upon first connection to a database it queries pg_type and
maps its internal data type handlers based on name. This allows it to avoid
hardcoding data type OIDs in source code, and easily handle user-defined
data types as well (enums, composites...). So a sudden rename of a datatype
would definitely cause a problem. Of course it's possible to first check
the server version and act accordingly but it seems to complicate things
needlessly.

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-10-17T19:38:18Z

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

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

Bruce Momjian <bruce@momjian.us> — 2016-10-18T23:25:21Z

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 +

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-10-25T05:38:56Z

On Tue, Oct 18, 2016 at 1:51 AM, Shay Rojansky <roji@roji.org> wrote:

> The current macaddr datatype needs to be kept for some time by renaming
>> it without changing OID and use the newer one for further usage.
>>
>
> From the point of view of a driver maintainer... Npgsql looks up data
> types by their name - upon first connection to a database it queries
> pg_type and maps its internal data type handlers based on name. This allows
> it to avoid hardcoding data type OIDs in source code, and easily handle
> user-defined data types as well (enums, composites...). So a sudden rename
> of a datatype would definitely cause a problem. Of course it's possible to
> first check the server version and act accordingly but it seems to
> complicate things needlessly.
>

Yes, that's correct. Changing the existing datatype name is a pain, but for
enhancing
its use to adopt the new hardware addresses, i feel this is required.

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.

Currently the patch lacks of documentation. Comments?

Regards,
Hari Babu
Fujitsu Australia

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-10-25T12:40:43Z

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

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

Robert Haas <robertmhaas@gmail.com> — 2016-10-26T18:45:15Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-11-04T00:09:28Z

On Tue, Oct 25, 2016 at 11:40 PM, Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

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

Yes. Before doing this change, it is better to confirm the approach and
then do all the changes.

1. Does everyone agrees that renaming the existing datatype without
changing the OID?

2. The old macaddress datatype rename to mac48 macaddr48
or macaddr6 or mac6.

3. Add the new datatype with the name that supports both 48 bit
and 64 bit MAC address.

4. The new datatype is of variable length datatype similar like INET,
so it can handle any future changes.

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

Thanks, I will do it from now onward.

Regards,
Hari Babu
Fujitsu Australia

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

Shay Rojansky <roji@roji.org> — 2016-11-04T08:55:26Z

>
> Yes. Before doing this change, it is better to confirm the approach and
> then do all the changes.
>
> 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.

For users who actually need the new variable-length type, it seems
perfectly reasonable to ask to switch to a new type - after all they're
making a change in their system. It would really be preferable to leave the
current type alone and create a new one.

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

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-11-05T00:19:09Z

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

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

Shay Rojansky <roji@roji.org> — 2016-11-06T21:40:38Z

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


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.

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-11-07T02:40:34Z

On Mon, Nov 7, 2016 at 8:40 AM, Shay Rojansky <roji@roji.org> 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?
>
>
> 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.

Regards,
Hari Babu
Fujitsu Australia

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

Shay Rojansky <roji@roji.org> — 2016-11-07T11:40:50Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-11-10T06:42:17Z

On Fri, Nov 4, 2016 at 11:09 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

>
>
> On Tue, Oct 25, 2016 at 11:40 PM, Peter Eisentraut <
> peter.eisentraut@2ndquadrant.com> wrote:
>
>> 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?
>
>
> Yes. Before doing this change, it is better to confirm the approach and
> then do all the changes.
>
> 1. Does everyone agrees that renaming the existing datatype without
> changing the OID?
>
> 2. The old macaddress datatype rename to mac48 macaddr48
> or macaddr6 or mac6.
>
> 3. Add the new datatype with the name that supports both 48 bit
> and 64 bit MAC address.
>
> 4. The new datatype is of variable length datatype similar like INET,
> so it can handle any future changes.
>

I didn't hear any problems with the approach in supporting the MACADDR
with 8 bytes storage. I will go with proposed design, and "mac48" as
the datatype name for the old macaddr.


Regards,
Hari Babu
Fujitsu Australia

On Fri, Nov 4, 2016 at 11:09 AM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

>
>
> On Tue, Oct 25, 2016 at 11:40 PM, Peter Eisentraut <
> peter.eisentraut@2ndquadrant.com> wrote:
>
>> 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?
>
>
> Yes. Before doing this change, it is better to confirm the approach and
> then do all the changes.
>
> 1. Does everyone agrees that renaming the existing datatype without
> changing the OID?
>
> 2. The old macaddress datatype rename to mac48 macaddr48
> or macaddr6 or mac6.
>
> 3. Add the new datatype with the name that supports both 48 bit
> and 64 bit MAC address.
>
> 4. The new datatype is of variable length datatype similar like INET,
> so it can handle any future changes.
>
>
>
>> > 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.
>
>
> Thanks, I will do it from now onward.
>
>
> Regards,
> Hari Babu
> Fujitsu Australia
>



-- 
Regards,
Hari Babu
Fujitsu Australia

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

Stephen Frost <sfrost@snowman.net> — 2016-11-19T19:29:42Z

* 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

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

Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-19T19:54:12Z

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

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

Robert Haas <robertmhaas@gmail.com> — 2016-11-21T18:33:43Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-11-22T04:46:32Z

On Tue, Nov 22, 2016 at 5:33 AM, Robert Haas <robertmhaas@gmail.com> wrote:

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

OK. Agreed.

Any suggestions for the name to be used for the new datatype the can
work for both 48 and 64 bit MAC addresses?

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

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.

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?

Regards,
Hari Babu
Fujitsu Australia

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

Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-22T14:42:44Z

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

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

Jon Nelson <jnelson+pgsql@jamponi.net> — 2016-11-22T17:37:26Z

On Tue, Nov 22, 2016 at 8:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

Be that as it may, but almost everybody else (outside the db world?) uses
bits.  The C types, for example, are expressed in bits (int8_t, int64_t,
etc...).

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

What's more, it now requires 2 comparisons and some logic versus the
possibility of a single memcmp.

-- 
Jon

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-11-22T23:52:22Z

On Wed, Nov 23, 2016 at 1:42 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

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?

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

Yes, that's correct. Both the above MAC addresses are different.

Sorry for not providing more details.

The new macaddr8 datatype can accept both 6 and 8 byte MAC addresses.
Let's assume the data in the table for the macaddr8 column as follows.

row1 = 01-01-01-02-02-02
row2 = 01-01-01-FF-FE-02-02-02

The MAC address is same, it is just a representation in 2 forms, one is 6
byte
and another is 8 byte.

What I am suggesting is, we can treat both of the rows data as same, because
it is just a representation difference.

To do the same, while comparing two MAC addresses that are of 6 and 8 byte
length, some special handling is added to check for the reserved keywords
in
the 8 byte MAC address, based on that the comparison is carried out.

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

As the new datatype that can store both 6 and 8 byte MAC addresses
as it is, while sending this data to client, based on the data that is
stored,
it sends 6 or 8 bytes.

If we go with storing 8 byte always, then sending all 8 bytes is fine.

Regards,
Hari Babu
Fujitsu Australia

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

Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-23T01:53:02Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-11-25T02:05:24Z

On Wed, Nov 23, 2016 at 12:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

Thanks for your feedback.

Here is attached updated patch with new datatype "macaddr8" with fixed
length
of 8 bytes.

If your input a 6 byte MAC address, it automatically  converts it into an 8
byte
MAC address by adding the reserved keywords and store it as an 8 byte
address.

While sending it to client it always send an 8 byte MAC address.

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.

Test and doc changes are also added.

comments?

Regards,
Hari Babu
Fujitsu Australia

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

Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-26T05:48:37Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-11-29T09:36:06Z

On Sat, Nov 26, 2016 at 4:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

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?

Regards,
Hari Babu
Fujitsu Australia

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-05T01:08:09Z

On Tue, Nov 29, 2016 at 8:36 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

>
>
> On Sat, Nov 26, 2016 at 4:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> 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.
>>
>
> 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.

Regards,
Hari Babu
Fujitsu Australia

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

Vitaly Burovoy <vitaly.burovoy@gmail.com> — 2017-01-06T04:51:07Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-09T08:15:39Z

On Fri, Jan 6, 2017 at 3:51 PM, Vitaly Burovoy <vitaly.burovoy@gmail.com>
wrote:

> 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:
>
>
Thanks for the review.


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

Updated the documentation to point to correct 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.
>

Fixed.


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

Changed accordingly.


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

Ok. Removed the above code that blocks the input.


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

Updated as per you suggestion.


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

Added the hint message to explain in detail about addresses that are
eligible for
conversion from macaddr8 type 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").
>

Done the indentation to correct the problem.


> 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



Corrected.

Updated patch is attached.

Regards,
Hari Babu
Fujitsu Australia

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

Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2017-01-14T07:28:10Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-19T02:16:05Z

On Sat, Jan 14, 2017 at 6:28 PM, Kuntal Ghosh <kuntalghosh.2007@gmail.com>
wrote:

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

Thanks for the review.

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

updated accordingly.


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

corrected.

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

Changed.


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

Changed by adding a typecast, because res is a double datatype value.


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

Corrected.


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

Corrected.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia

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

Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2017-01-23T07:29:32Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-24T04:55:08Z

On Mon, Jan 23, 2017 at 6:29 PM, Kuntal Ghosh <kuntalghosh.2007@gmail.com>
wrote:

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

Thanks for the review.

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

Added the support for macaddr8 datatype for both btree_gin and btree_gist
modules and the tests also.

I didn't update the *.sql files to the latest version and just added the
update *.sql
file only as like earlier changes on btree_gist for UUID support to make it
easier
for the reviewer.

The patch is split into two parts.
1. Macaddr8 datatype support
2. Contrib module support.

Regards,
Hari Babu
Fujitsu Australia

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

Vitaly Burovoy <vitaly.burovoy@gmail.com> — 2017-01-25T07:43:27Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-25T12:30:56Z

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

Changed accordingly.


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


Corrected as suggested.

Updated patch attached. There is no change in the contrib patch.

Regards,
Hari Babu
Fujitsu Australia

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

Vitaly Burovoy <vitaly.burovoy@gmail.com> — 2017-01-26T10:42:18Z

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

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

Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2017-01-27T07:10:36Z

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 extension
 comment = '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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-27T15:50:01Z

On Thu, Jan 26, 2017 at 9:42 PM, Vitaly Burovoy <vitaly.burovoy@gmail.com>
wrote:

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


Thanks for the review. Corrected in the latest patch.

Regards,
Hari Babu
Fujitsu Australia

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-27T15:52:34Z

On Fri, Jan 27, 2017 at 6:10 PM, Kuntal Ghosh <kuntalghosh.2007@gmail.com>
wrote:

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

Corrected.


> 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 extension
>  comment = '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.


Corrected the change in btree_gin.control file also.
Thanks for the review.

Updated patches are attached.


Regards,
Hari Babu
Fujitsu Australia

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

Vitaly Burovoy <vitaly.burovoy@gmail.com> — 2017-01-31T05:13:15Z

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

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

Michael Paquier <michael.paquier@gmail.com> — 2017-01-31T06:43:05Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-31T12:39:53Z

On Tue, Jan 31, 2017 at 4:13 PM, Vitaly Burovoy <vitaly.burovoy@gmail.com>
wrote:

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


Thanks for the review.


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

I think I might have missed to update during OID number changes.
Fixed.


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

Corrected.


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

Corrected.


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

Done.

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;


Done.

Updated patches are attached.


Regards,
Hari Babu
Fujitsu Australia

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

Vitaly Burovoy <vitaly.burovoy@gmail.com> — 2017-01-31T19:27:03Z

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

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

Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2017-02-01T05:01:54Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-02-06T01:27:12Z

On Wed, Feb 1, 2017 at 6:27 AM, Vitaly Burovoy <vitaly.burovoy@gmail.com>
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: [REVIEW] macaddr 64 bit (EUI-64) datatype support

Stephen Frost <sfrost@snowman.net> — 2017-03-09T22:41:48Z

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

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

Stephen Frost <sfrost@snowman.net> — 2017-03-12T19:38:58Z

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

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

Stephen Frost <sfrost@snowman.net> — 2017-03-12T19:52:30Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-03-14T01:27:36Z

On Mon, Mar 13, 2017 at 6:38 AM, Stephen Frost <sfrost@snowman.net> wrote:

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

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: [REVIEW] macaddr 64 bit (EUI-64) datatype support

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-03-14T01:34:12Z

On Mon, Mar 13, 2017 at 6:52 AM, Stephen Frost <sfrost@snowman.net> wrote:

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

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: [REVIEW] macaddr 64 bit (EUI-64) datatype support

Stephen Frost <sfrost@snowman.net> — 2017-03-15T15:20:53Z

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

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

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-03-16T00:57:12Z

On Thu, Mar 16, 2017 at 2:20 AM, Stephen Frost <sfrost@snowman.net> 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!).
>

Thanks for the review and changes.

Regards,
Hari Babu
Fujitsu Australia

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

Andres Freund <andres@anarazel.de> — 2017-03-16T01:12:23Z

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 range of data type [-Wtype-limits]
  if (*ptr < 0 || *ptr > 127)
                       ^
Andres

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

Stephen Frost <sfrost@snowman.net> — 2017-03-16T01:19:19Z

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

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

Stephen Frost <sfrost@snowman.net> — 2017-03-16T03:24:41Z

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