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

From: Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
To: Vitaly Burovoy <vitaly(dot)burovoy(at)gmail(dot)com>
Cc: Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Shay Rojansky <roji(at)roji(dot)org>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Julien Rouhaud <julien(dot)rouhaud(at)dalibo(dot)com>, Craig Ringer <craig(at)2ndquadrant(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: macaddr 64 bit (EUI-64) datatype support
Date: 2017-01-25 12:30:56
Message-ID: CAJrrPGfxhFk84SMWvo8q65KncjANN0cZbMq-KDSPe6exjqdwRA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On Wed, Jan 25, 2017 at 6:43 PM, Vitaly Burovoy <vitaly(dot)burovoy(at)gmail(dot)com>
wrote:

> On 1/23/17, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> wrote:
> > The patch is split into two parts.
> > 1. Macaddr8 datatype support
> > 2. Contrib module support.
>
> Hello,
>
> I'm sorry for the delay.
> The patch is almost done, but I have two requests since the last review.
>

Thanks for the review.

> 1.
> src/backend/utils/adt/mac8.c:
> + int a,
> + b,
> + c,
> + d = 0,
> + e = 0,
> ...
>
> There is no reason to set them as 0. For EUI-48 they will be
> reassigned in the "if (count != 8)" block, for EUI-64 -- in one of
> sscanf.
> They could be set to "d = 0xFF, e = 0xFE," and avoid the "if" block
> mentioned above, but it makes the code be much less readable.
>
> Oh. I see. In the current version it must be assigned because for
> EUI-48 memory can have values <0 or >255 in uninitialized variables.
> It is better to avoid initialization by merging two parts of the code:
> + if (count != 6)
> + {
> + /* May be a 8-byte MAC address */
> ...
> + if (count != 8)
> + {
> + d = 0xFF;
> + e = 0xFE;
> + }
>
> to a single one:
> + if (count == 6)
> + {
> + d = 0xFF;
> + e = 0xFE;
> + }
> + else
> + {
> + /* May be a 8-byte MAC address */
> ...
>

Changed accordingly.

> 2.
> src/backend/utils/adt/network.c:
> + res = (mac->a << 24) | (mac->b << 16) |
> (mac->c << 8) | (mac->d);
> + res = (double)((uint64)res << 32);
> + res += (mac->e << 24) | (mac->f << 16) |
> (mac->g << 8) | (mac->h);
>
> Khm... I trust that modern compilers can do a lot of optimizations but
> for me it looks terrible because of needless conversions.
> The reason why earlier versions did have two lines "res *= 256 * 256"
> was an integer overflow for four multipliers, but it can be solved by
> defining the first multiplier as a double:
> + res = (mac->a << 24) | (mac->b << 16) |
> (mac->c << 8) | (mac->d);
> + res *= (double)256 * 256 * 256 * 256;
> + res += (mac->e << 24) | (mac->f << 16) |
> (mac->g << 8) | (mac->h);
>
> In this case the left-hand side argument for the "*=" operator is
> computed at the compile time as a single constant.
> The second line can be written as "res *= 256. * 256 * 256 * 256;"
> (pay attention to a dot in the first multiplier), but it is not
> readable at all (and produces the same code).

Corrected as suggested.

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

Regards,
Hari Babu
Fujitsu Australia

Attachment Content-Type Size
mac_eui64_support_8.patch application/octet-stream 46.9 KB
contrib_macaddr8_1.patch application/octet-stream 20.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2017-01-25 12:56:32 Re: multivariate statistics (v19)
Previous Message Rahila Syed 2017-01-25 12:18:54 Re: Improvements in psql hooks for variables