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

[Haribabu Kommi]

On Tue, Jan 31, 2017 at 4:13 PM, Vitaly Burovoy 
wrote:

> On 1/27/17, Haribabu Kommi  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


contrib_macaddr8_3.patch
Description: Binary data


mac_eui64_support_10.patch
Description: Binary data

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

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

[Michael Paquier]

On Tue, Jan 31, 2017 at 2:13 PM, Vitaly Burovoy
 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


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

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

[Vitaly Burovoy]

On 1/27/17, Haribabu Kommi  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


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

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

[Haribabu Kommi]

On Fri, Jan 27, 2017 at 6:10 PM, Kuntal Ghosh 
wrote:

> On Wed, Jan 25, 2017 at 6:00 PM, Haribabu Kommi
>  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


mac_eui64_support_9.patch
Description: Binary data


contrib_macaddr8_2.patch
Description: Binary data

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

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

[Haribabu Kommi]

On Thu, Jan 26, 2017 at 9:42 PM, Vitaly Burovoy 
wrote:

> On 1/25/17, Haribabu Kommi  wrote:
> > On Wed, Jan 25, 2017 at 6:43 PM, Vitaly Burovoy <
> vitaly.buro...@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: [HACKERS] macaddr 64 bit (EUI-64) datatype support

[Kuntal Ghosh]

On Wed, Jan 25, 2017 at 6:00 PM, Haribabu Kommi
 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


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

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

[Vitaly Burovoy]

On 1/25/17, Haribabu Kommi  wrote:
> On Wed, Jan 25, 2017 at 6:43 PM, Vitaly Burovoy 
> wrote:
>
>> On 1/23/17, Haribabu Kommi  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.
>>



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


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

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

[Haribabu Kommi]

On Wed, Jan 25, 2017 at 6:43 PM, Vitaly Burovoy 
wrote:

> On 1/23/17, Haribabu Kommi  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


mac_eui64_support_8.patch
Description: Binary data


contrib_macaddr8_1.patch
Description: Binary data

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

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

[Vitaly Burovoy]

On 1/23/17, Haribabu Kommi  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


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

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

[Haribabu Kommi]

On Mon, Jan 23, 2017 at 6:29 PM, Kuntal Ghosh 
wrote:

> On Thu, Jan 19, 2017 at 7:46 AM, Haribabu Kommi
>  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


mac_eui64_support_7.patch
Description: Binary data


contrib_macaddr8_1.patch
Description: Binary data

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

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

[Kuntal Ghosh]

On Thu, Jan 19, 2017 at 7:46 AM, Haribabu Kommi
 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


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

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

[Haribabu Kommi]

On Sat, Jan 14, 2017 at 6:28 PM, Kuntal Ghosh 
wrote:

> On Mon, Jan 9, 2017 at 1:45 PM, Haribabu Kommi 
> 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


mac_eui64_support_6.patch
Description: Binary data

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

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

[Kuntal Ghosh]

On Mon, Jan 9, 2017 at 1:45 PM, Haribabu Kommi  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


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

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

[Haribabu Kommi]

On Fri, Jan 6, 2017 at 3:51 PM, Vitaly Burovoy 
wrote:

> On 1/4/17, Haribabu Kommi  wrote:
> > On Tue, Nov 29, 2016 at 8:36 PM, Haribabu Kommi <
> kommi.harib...@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.
> + trunc(macaddr8) 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",
> +  , , , , , , 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 7744123 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 

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

[Vitaly Burovoy]

On 1/4/17, Haribabu Kommi  wrote:
> On Tue, Nov 29, 2016 at 8:36 PM, Haribabu Kommi 
> 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.
+ trunc(macaddr8) 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",
+  , , , , , , 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 7744123 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_+=fvo6oz4tgv1kkhmom11ankihbokpuzki1ca...@mail.gmail.com
[2]http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1817.htm


-- 
Best regards,
Vitaly Burovoy


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

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

[Haribabu Kommi]

On Tue, Nov 29, 2016 at 8:36 PM, Haribabu Kommi 
wrote:

>
>
> On Sat, Nov 26, 2016 at 4:48 PM, Tom Lane  wrote:
>
>> Haribabu Kommi  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


mac_eui64_support_4.patch
Description: Binary data

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

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

[Haribabu Kommi]

On Sat, Nov 26, 2016 at 4:48 PM, Tom Lane  wrote:

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


mac_eui64_support_3.patch
Description: Binary data

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

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

[Tom Lane]

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


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

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

[Haribabu Kommi]

On Wed, Nov 23, 2016 at 12:53 PM, Tom Lane  wrote:

> Haribabu Kommi  writes:
> > On Wed, Nov 23, 2016 at 1:42 AM, Tom Lane  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


mac_eui64_support_2.patch
Description: Binary data

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

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

[Tom Lane]

Haribabu Kommi  writes:
> On Wed, Nov 23, 2016 at 1:42 AM, Tom Lane  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


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

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

[Haribabu Kommi]

On Wed, Nov 23, 2016 at 1:42 AM, Tom Lane  wrote:

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

[Jon Nelson]

On Tue, Nov 22, 2016 at 8:42 AM, Tom Lane  wrote:

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

[Tom Lane]

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


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

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

[Haribabu Kommi]

On Tue, Nov 22, 2016 at 5:33 AM, Robert Haas  wrote:

> On Sat, Nov 19, 2016 at 2:54 PM, Tom Lane  wrote:
> > Stephen Frost  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: [HACKERS] macaddr 64 bit (EUI-64) datatype support

[Robert Haas]

On Sat, Nov 19, 2016 at 2:54 PM, Tom Lane  wrote:
> Stephen Frost  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


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

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

[Tom Lane]

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


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

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

[Stephen Frost]

* Shay Rojansky (r...@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


signature.asc
Description: Digital signature

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

[Haribabu Kommi]

On Fri, Nov 4, 2016 at 11:09 AM, Haribabu Kommi 
wrote:

>
>
> On Tue, Oct 25, 2016 at 11:40 PM, Peter Eisentraut <
> peter.eisentr...@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 
wrote:

>
>
> On Tue, Oct 25, 2016 at 11:40 PM, Peter Eisentraut <
> peter.eisentr...@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: [HACKERS] macaddr 64 bit (EUI-64) datatype support

[Shay Rojansky]

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

[Haribabu Kommi]

On Mon, Nov 7, 2016 at 8:40 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?
>
>
> 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: [HACKERS] macaddr 64 bit (EUI-64) datatype support

[Shay Rojansky]

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

[Peter Eisentraut]

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


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

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

[Shay Rojansky]

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

[Haribabu Kommi]

On Tue, Oct 25, 2016 at 11:40 PM, Peter Eisentraut <
peter.eisentr...@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: [HACKERS] macaddr 64 bit (EUI-64) datatype support

[Robert Haas]

On Tue, Oct 18, 2016 at 7:25 PM, Bruce Momjian  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


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

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

[Peter Eisentraut]

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


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

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

[Haribabu Kommi]

On Tue, Oct 18, 2016 at 1:51 AM, Shay Rojansky  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


mac_eui64_support_1.patch
Description: Binary data

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

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

[Bruce Momjian]

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


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

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

[Peter Eisentraut]

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


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

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

[Shay Rojansky]

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

[Haribabu Kommi]

On Thu, Oct 13, 2016 at 4:10 PM, Vitaly Burovoy 
wrote:

> On 10/12/16, Tom Lane  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: [HACKERS] macaddr 64 bit (EUI-64) datatype support

[Vitaly Burovoy]

On 10/12/16, Tom Lane  wrote:
> Alvaro Herrera  writes:
>> Tom Lane wrote:
>>> Vitaly Burovoy  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 

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

[Haribabu Kommi]

On Thu, Oct 13, 2016 at 7:59 AM, Tom Lane  wrote:

> Alvaro Herrera  writes:
> > Tom Lane wrote:
> >> Vitaly Burovoy  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: [HACKERS] macaddr 64 bit (EUI-64) datatype support

[Haribabu Kommi]

On Thu, Oct 13, 2016 at 7:31 AM, Vitaly Burovoy 
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: [HACKERS] macaddr 64 bit (EUI-64) datatype support

[Vitaly Burovoy]

On 10/12/16, Tom Lane  wrote:
> Vitaly Burovoy  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


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

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

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Vitaly Burovoy  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


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

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

[Tom Lane]

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


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

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

[Alvaro Herrera]

Tom Lane wrote:
> Vitaly Burovoy  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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

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

[Tom Lane]

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


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

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

[Vitaly Burovoy]

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

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

can not be rewritten as:

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

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

-- 
Best regards,
Vitaly Burovoy


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

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

[Vitaly Burovoy]

On 10/12/16, Vitaly Burovoy  wrote:
> On 10/12/16, Alvaro Herrera  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


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

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

[Vitaly Burovoy]

On 10/12/16, Alvaro Herrera  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


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

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

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

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

[Julien Rouhaud]

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


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

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

[Tom Lane]

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


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

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

[Alvaro Herrera]

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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

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

[Julien Rouhaud]

On 12/10/2016 09:32, Craig Ringer wrote:
> On 12 October 2016 at 14:30, Haribabu Kommi  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


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

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

[Craig Ringer]

On 12 October 2016 at 14:30, Haribabu Kommi  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


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

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

[Michael Paquier]

On Wed, Oct 12, 2016 at 3:30 PM, Haribabu Kommi
 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


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