Cleaning up the INET/CIDR mess
We've had previous discussions about how the distinction between INET
and CIDR isn't very well thought out, for instance
http://archives.postgresql.org/pgsql-hackers/2005-01/msg01021.php
http://archives.postgresql.org/pgsql-hackers/2006-01/msg00233.php
The basic problem is that the code is schizophrenic about whether these
types are "the same" or not. The fact that we have implicit binary
(no function) coercions in both directions makes them effectively the
same, so why are there two different type names in the catalogs?
On the other hand, if they should be different (and they definitely
have different I/O behavior), this coercion behavior is wrong. Also,
if they are different types, it's redundant to have a flag inside the
data structure saying which type a particular value is.
After some consideration I've come to the conclusion that we really do
want them to be separate types: the I/O behavior is settled (after quite
some long discussions) and we don't want to change it, so we can't merge
them into one type. That leads to the following proposals:
Remove the internal is_cidr flag; it's a waste of space. (It doesn't
actually cost anything today, because of alignment considerations, but
it would cost 2 bytes if we implement the proposed 2-byte-length-word
variant datum format.) Even more to the point, the presence of the
flag has encouraged the sort of sloppy thinking and coding that got us
into this mess. Whether it's an INET or a CIDR should be totally
determined by the SQL type system.
Without the flag, it's okay for cidr-to-inet to be a binary-compatible (no
function) conversion. However, inet-to-cidr has to either zero out bits
to the right of the netmask, or error out if any are set. Joachim Wieland
posted a patch that makes the coercion function just silently zero out any
such bits. That's OK with me, but does anyone want to argue for an error?
(If we do make the coercion function raise error, then we'd probably need
to provide a separate function that supports the bit-zeroing conversion.)
Currently, both directions of cast are implicit, but that is a bad idea.
I propose keeping cidr-to-inet as implicit but making inet-to-cidr an
assignment cast. This fits with the fact that inet can represent all
values of cidr but not vice versa (compare int4 and int8).
Given the implicit binary-compatible coercion, it's OK to have just a
single function taking inet for any case where the function truly doesn't
care if it's looking at inet or cidr input. For the cases where the code
currently pays attention to is_cidr, we'd have to make two separate
functions. AFAICT that's only abbrev(inet) and text(inet) among the
current functions. Also, set_masklen(inet,integer) would have to come
in two flavors since the output type should be the same as the input.
The relationship of cidr and inet would be a little bit like the relation
between varchar and text. For instance, varchar doesn't have any
comparison operators of its own, but piggybacks on text's comparison
operators, relying on the implicit cast from varchar to text to make this
transparent to users.
One other point is what to do with the binary I/O functions (send/receive)
for inet and cidr. I think that we should continue to send the is_cidr
flag byte for backwards-compatibility reasons. On receive, we could
either ignore that byte entirely, or insist that it match the expected
datatype. I'm inclined to ignore the byte but am willing to listen to
arguments to raise an error instead.
Comments?
regards, tom lane
On Tue, Jan 24, 2006 at 01:23:17PM -0500, Tom Lane wrote:
Without the flag, it's okay for cidr-to-inet to be a binary-compatible (no
function) conversion. However, inet-to-cidr has to either zero out bits
to the right of the netmask, or error out if any are set. Joachim Wieland
posted a patch that makes the coercion function just silently zero out any
such bits. That's OK with me, but does anyone want to argue for an error?
(If we do make the coercion function raise error, then we'd probably need
to provide a separate function that supports the bit-zeroing conversion.)
I'd argue for an error, on correctness grounds (someone's bound to
come back having misused these, and complain that it silently changed
data. They'd have a point).
One other point is what to do with the binary I/O functions (send/receive)
for inet and cidr. I think that we should continue to send the is_cidr
flag byte for backwards-compatibility reasons. On receive, we could
either ignore that byte entirely, or insist that it match the expected
datatype. I'm inclined to ignore the byte but am willing to listen to
arguments to raise an error instead.
If this is exposed to users in some way (I don't think it is, is it?)
then I'd argue for erroring, on the same grounds of what I say above.
But otherwise, I think you could ignore it.
A
--
Andrew Sullivan | ajs@crankycanuck.ca
I remember when computers were frustrating because they *did* exactly what
you told them to. That actually seems sort of quaint now.
--J.D. Baldwin
On 2006-01-24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Without the flag, it's okay for cidr-to-inet to be a binary-compatible (no
function) conversion. However, inet-to-cidr has to either zero out bits
to the right of the netmask, or error out if any are set. Joachim Wieland
posted a patch that makes the coercion function just silently zero out any
such bits. That's OK with me, but does anyone want to argue for an error?
(If we do make the coercion function raise error, then we'd probably need
to provide a separate function that supports the bit-zeroing conversion.)Currently, both directions of cast are implicit, but that is a bad idea.
I propose keeping cidr-to-inet as implicit but making inet-to-cidr an
assignment cast. This fits with the fact that inet can represent all
values of cidr but not vice versa (compare int4 and int8).
If inet-to-cidr can zero out bits silently, then wouldn't making it an
assignment cast be rather dangerous and error-prone?
Given the implicit binary-compatible coercion, it's OK to have just a
single function taking inet for any case where the function truly doesn't
care if it's looking at inet or cidr input. For the cases where the code
currently pays attention to is_cidr, we'd have to make two separate
functions. AFAICT that's only abbrev(inet) and text(inet) among the
current functions. Also, set_masklen(inet,integer) would have to come
in two flavors since the output type should be the same as the input.
You sometimes need set_masklen(cidr,integer) returning inet, and I'd bet
there's existing code that does that.
The relationship of cidr and inet would be a little bit like the relation
between varchar and text. For instance, varchar doesn't have any
comparison operators of its own, but piggybacks on text's comparison
operators, relying on the implicit cast from varchar to text to make this
transparent to users.
Well, inet/cidr have far more justification for being separate types than
text/varchar do - the text/varchar issue causes a great deal of confusion.
--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services
This is exactly what I had in mind:
split the types
zero out the bits going to cidr
no change going to inet
make functions take inet, which as not cast change
---------------------------------------------------------------------------
Tom Lane wrote:
We've had previous discussions about how the distinction between INET
and CIDR isn't very well thought out, for instance
http://archives.postgresql.org/pgsql-hackers/2005-01/msg01021.php
http://archives.postgresql.org/pgsql-hackers/2006-01/msg00233.phpThe basic problem is that the code is schizophrenic about whether these
types are "the same" or not. The fact that we have implicit binary
(no function) coercions in both directions makes them effectively the
same, so why are there two different type names in the catalogs?
On the other hand, if they should be different (and they definitely
have different I/O behavior), this coercion behavior is wrong. Also,
if they are different types, it's redundant to have a flag inside the
data structure saying which type a particular value is.After some consideration I've come to the conclusion that we really do
want them to be separate types: the I/O behavior is settled (after quite
some long discussions) and we don't want to change it, so we can't merge
them into one type. That leads to the following proposals:Remove the internal is_cidr flag; it's a waste of space. (It doesn't
actually cost anything today, because of alignment considerations, but
it would cost 2 bytes if we implement the proposed 2-byte-length-word
variant datum format.) Even more to the point, the presence of the
flag has encouraged the sort of sloppy thinking and coding that got us
into this mess. Whether it's an INET or a CIDR should be totally
determined by the SQL type system.Without the flag, it's okay for cidr-to-inet to be a binary-compatible (no
function) conversion. However, inet-to-cidr has to either zero out bits
to the right of the netmask, or error out if any are set. Joachim Wieland
posted a patch that makes the coercion function just silently zero out any
such bits. That's OK with me, but does anyone want to argue for an error?
(If we do make the coercion function raise error, then we'd probably need
to provide a separate function that supports the bit-zeroing conversion.)Currently, both directions of cast are implicit, but that is a bad idea.
I propose keeping cidr-to-inet as implicit but making inet-to-cidr an
assignment cast. This fits with the fact that inet can represent all
values of cidr but not vice versa (compare int4 and int8).Given the implicit binary-compatible coercion, it's OK to have just a
single function taking inet for any case where the function truly doesn't
care if it's looking at inet or cidr input. For the cases where the code
currently pays attention to is_cidr, we'd have to make two separate
functions. AFAICT that's only abbrev(inet) and text(inet) among the
current functions. Also, set_masklen(inet,integer) would have to come
in two flavors since the output type should be the same as the input.The relationship of cidr and inet would be a little bit like the relation
between varchar and text. For instance, varchar doesn't have any
comparison operators of its own, but piggybacks on text's comparison
operators, relying on the implicit cast from varchar to text to make this
transparent to users.One other point is what to do with the binary I/O functions (send/receive)
for inet and cidr. I think that we should continue to send the is_cidr
flag byte for backwards-compatibility reasons. On receive, we could
either ignore that byte entirely, or insist that it match the expected
datatype. I'm inclined to ignore the byte but am willing to listen to
arguments to raise an error instead.Comments?
regards, tom lane
---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Andrew - Supernews <andrew+nonews@supernews.com> writes:
If inet-to-cidr can zero out bits silently, then wouldn't making it an
assignment cast be rather dangerous and error-prone?
The proposal was to make cidr-to-inet an implicit cast (happens automatically
without being requested) but make cidr-to-inet not implicit (you need to type
myinet::cidr or the "CAST" syntax to get it to happen).
Making the cast zero bits or making it error and having a separate function to
handle the conversion both seem entirely reasonable approaches.
I wonder if this would be an opportunity to fix Postgres's handling of
addresses like '10.1'. The standard interpretation of this is the same as
'10.0.0.1'. Currently Postgres interprets it as '10.1.0.0' for cidr and gives
an error for inet.
Perhaps if they become truly separate types then we could leave the cidr
behaviour alone since it is common to write things like 10.1/16 to refer to
the network prefix. But let inet have the standard interpretation.
It might be confusing but actually it's entirely logical. cidr is a type for
representing address ranges. '10.1/16'::cidr represents a range of addresses
from 10.1.0.0-10.1.255.255. There would be no reasonable use case for 10.0.0.1
in a cidr unless it was /32 which would refer to a specific address, in which
case it would make more sense to use an inet.
On the other hand '10.1'::inet represents a specific address and it's clear
which address it means -- it's even required to be interpreted this way by the
POSIX functions. Erroring out as Postgres currently does means applications
built on Postgres don't accept standard inernet address syntax.
--
greg
Greg Stark <gsstark@mit.edu> writes:
I wonder if this would be an opportunity to fix Postgres's handling of
addresses like '10.1'.
You've mistaken this for a proposal to change the I/O behavior, which
it is specifically not.
The standard interpretation of this is the same as '10.0.0.1'.
Standard according to whom? Paul Vixie evidently doesn't think that
that's a standard abbreviation, else the code we borrowed from libbind
would do it already.
regards, tom lane
Tom Lane wrote:
Greg Stark <gsstark@mit.edu> writes:
I wonder if this would be an opportunity to fix Postgres's handling
of addresses like '10.1'.You've mistaken this for a proposal to change the I/O behavior, which
it is specifically not.The standard interpretation of this is the same as '10.0.0.1'.
Standard according to whom? Paul Vixie evidently doesn't think that
that's a standard abbreviation, else the code we borrowed from libbind
would do it already.
We had a **LONG** discussion on the I/O formats back in the 7.2 timeframe.
the current
behavior is the result of that.
Please do **NOT** change the I/O formats.
LER
--
Larry Rosenman http://www.lerctr.org/~ler
Phone: +1 512-248-2683 E-Mail: ler@lerctr.org
US Mail: 430 Valona Loop, Round Rock, TX 78681-3893
Tom Lane wrote:
Greg Stark <gsstark@mit.edu> writes:
I wonder if this would be an opportunity to fix Postgres's handling of
addresses like '10.1'.You've mistaken this for a proposal to change the I/O behavior, which
it is specifically not.The standard interpretation of this is the same as '10.0.0.1'.
Standard according to whom? Paul Vixie evidently doesn't think that
that's a standard abbreviation, else the code we borrowed from libbind
would do it already.
Agreed. 10.1 as 10.0.0.1 is an old behavior which has been removed from
most modern versions of networking tools.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073On 2006-01-25, Bruce Momjian <pgman@candle.pha.pa.us> wrote:
Agreed. 10.1 as 10.0.0.1 is an old behavior which has been removed from
most modern versions of networking tools.
Indeed so. However the current behaviour has neither the merit of being
traditional nor the merit of being logical:
=> select '10.1'::cidr;
cidr
-------------
10.1.0.0/16
(1 row)
=> select '128.1'::cidr;
cidr
--------------
128.1.0.0/16
(1 row)
=> select '192.1'::cidr;
cidr
--------------
192.1.0.0/24
(1 row)
Having the behaviour be dependent on which part of the IP space is used
is a total nonsense on the modern, CIDR, internet! The C in CIDR even
stands for "Classless", so how can you ever justify introducing _new_,
non-traditional, dependencies on the traditional classes?
--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services
On Jan 25, 2006, at 9:29 AM, Bruce Momjian wrote:
Tom Lane wrote:
Greg Stark <gsstark@mit.edu> writes:
I wonder if this would be an opportunity to fix Postgres's
handling of
addresses like '10.1'.You've mistaken this for a proposal to change the I/O behavior, which
it is specifically not.The standard interpretation of this is the same as '10.0.0.1'.
Standard according to whom? Paul Vixie evidently doesn't think that
that's a standard abbreviation, else the code we borrowed from
libbind
would do it already.Agreed. 10.1 as 10.0.0.1 is an old behavior which has been removed
from
most modern versions of networking tools.
Whether PG should support it or not is another question (personally I
think
that anything other than a dotted quad should fail with an error) but
it certainly
hasn't been removed from most modern versions of networking tools.
gethostbyname() is used by most networking tools, and on most unix OSes
it believes "10.1" 'resolves to' "10.0.0.1". That includes current
versions of
linux, OS X, Solaris, Windows XP and I believe the BSDs.
So the vast majority of applications on the vast majority of deployed
platforms
believe that "10.1" is the address 10.0.0.1. (As is often the case binds
behaviour is inconsistent and can't really be used as "proof" of
standard
behaviour).
Cheers,
Steve
Andrew - Supernews wrote:
On 2006-01-25, Bruce Momjian <pgman@candle.pha.pa.us> wrote:
Agreed. 10.1 as 10.0.0.1 is an old behavior which has been removed from
most modern versions of networking tools.Indeed so. However the current behaviour has neither the merit of being
traditional nor the merit of being logical:=> select '10.1'::cidr;
cidr
-------------
10.1.0.0/16
(1 row)=> select '128.1'::cidr;
cidr
--------------
128.1.0.0/16
(1 row)=> select '192.1'::cidr;
cidr
--------------
192.1.0.0/24
(1 row)Having the behaviour be dependent on which part of the IP space is used
is a total nonsense on the modern, CIDR, internet! The C in CIDR even
stands for "Classless", so how can you ever justify introducing _new_,
non-traditional, dependencies on the traditional classes?
This is coming from inet_net_pton.c, which we got from:
* Copyright (c) 2004 by Internet Systems Consortium, Inc. ("ISC")
* Copyright (c) 1996,1999 by Internet Software Consortium.
...
if (bits == -1)
{
if (*odst >= 240) /* Class E */
bits = 32;
else if (*odst >= 224) /* Class D */
bits = 8;
else if (*odst >= 192) /* Class C */
bits = 24;
else if (*odst >= 128) /* Class B */
bits = 16;
else
/* Class A */
bits = 8;
/* If imputed mask is narrower than specified octets, widen. */
if (bits < ((dst - odst) * 8))
bits = (dst - odst) * 8;
...
test=> select '11'::cidr;
cidr
------------
11.0.0.0/8
(1 row)
We are doing our best here to follow industry standards on how things
should behave.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Andrew - Supernews <andrew+nonews@supernews.com> writes:
On 2006-01-25, Bruce Momjian <pgman@candle.pha.pa.us> wrote:
Agreed. 10.1 as 10.0.0.1 is an old behavior which has been removed from
most modern versions of networking tools.
On the contrary not only is it still widely used but it is *required* by POSIX
for the relevant functions, inet_aton and getaddrinfo. Note that getaddrinfo
was created from whole cloth by POSIX so there was no backwards compatibility
need for it.
This isn't an obscure old-fashioned thing. People really do use this syntax.
Indeed so. However the current behaviour has neither the merit of being
traditional nor the merit of being logical:
Well for networks (cidr datatype) people do frequently refer to things like
10.1/16 and intend it to mean the network prefix. Sure you could argue having
the netmask default to the old class-based addressing is anachronistic but
what other default netmask would you suggest anyways? The only other
reasonable default would be the longest 0-bit suffix which would produce some
odd surprising results like '10.1/16' but '10.2/17'.
--
greg
On 2006-01-25, Bruce Momjian <pgman@candle.pha.pa.us> wrote:
Andrew - Supernews wrote:
Having the behaviour be dependent on which part of the IP space is used
is a total nonsense on the modern, CIDR, internet! The C in CIDR even
stands for "Classless", so how can you ever justify introducing _new_,
non-traditional, dependencies on the traditional classes?This is coming from inet_net_pton.c, which we got from:
That the behaviour comes from ISC code does not make it correct.
We are doing our best here to follow industry standards on how things
should behave.
This is not an "industry standard".
--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services
I have a question in a different direction. What is the meaning of the network
mask in the inet data type anyways? Hosts don't have network masks, only
networks.
If we could store inet in four bytes it would be vastly more efficient both in
disk space usage and in cpu at runtime.
I think it would also clear up the perpetual user confusion between the two
datatypes. I posit that the main source of the confusion is that currently
Postgres lets you use inet for everything, even if what you're really storing
is a network address range which is what the cidr datatype is really for.
--
greg
On 2006-01-25, Greg Stark <gsstark@mit.edu> wrote:
This isn't an obscure old-fashioned thing. People really do use this syntax.
Given how little code now supports 10.1 meaning 10.0.0.1, that seems a
questionable point.
Indeed so. However the current behaviour has neither the merit of being
traditional nor the merit of being logical:Well for networks (cidr datatype) people do frequently refer to things like
10.1/16 and intend it to mean the network prefix.
Do you mean they refer to '10.1' and intend it to mean '10.1/16'? If so I
agree; but in that case, not only should '10.1' mean '10.1/16', but also
'192.1' should mean '192.1/16' and _NOT_ '192.1/24'.
Sure you could argue having
the netmask default to the old class-based addressing is anachronistic but
what other default netmask would you suggest anyways?
The one implied by the number of octets specified, assuming you are going
to accept the abbreviated forms at all.
(FWIW, ip4r at this time does not even accept '10.1/16', it insists on
'10.1.0.0/16'.)
--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services
* Greg Stark (gsstark@mit.edu) wrote:
I have a question in a different direction. What is the meaning of the network
mask in the inet data type anyways? Hosts don't have network masks, only
networks.If we could store inet in four bytes it would be vastly more efficient both in
disk space usage and in cpu at runtime.I think it would also clear up the perpetual user confusion between the two
datatypes. I posit that the main source of the confusion is that currently
Postgres lets you use inet for everything, even if what you're really storing
is a network address range which is what the cidr datatype is really for.
I wholeheartedly agree with this. It also makes the only cast option
from inet to cidr to be with a /32 and thus there's no zeroing of bits.
It would then be nice to have a function to which you pass in a cidr and
a netmask and say "give me the CIDR this CIDR is in with this mask".
With the inet-to-cidr implicit cast you could use this function to get
the CIDR you want without having to worry about the implicit cast
throwing data away. The cidr-to-inet is then throwing away the mask and
so I'm not sure we should have an implicit cast in that direction but
honestly I wouldn't complain if we did.
Thanks,
Stephen
On 2006-01-25, Greg Stark <gsstark@MIT.EDU> wrote:
I have a question in a different direction. What is the meaning of the
network mask in the inet data type anyways? Hosts don't have network masks,
only networks.
As far as I can tell, the "inet" semantics are supposed to represent a
network interface, rather than just an address. So it designates a network
and a host within that network. This is a significant semantic overload,
which is not relevent to many applications and may be counterproductive
(for example, if you had a database of hosts and networks, the network info
would more correctly be accessed via a reference to a separate table than
embedded in the host address).
If we could store inet in four bytes it would be vastly more efficient both
in disk space usage and in cpu at runtime.
That's not reasonable for inet/cidr due to the need to support ipv6.
If you want that for ip4-only apps, that's why pgfoundry.org/projects/ip4r
exists. It is possible that ip4r will be extended to ipv6 addresses, but
most unlikely that it will ever implement the overloaded "inet" semantics.
--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services
On Jan 25, 2006, at 10:30 AM, Andrew - Supernews wrote:
On 2006-01-25, Greg Stark <gsstark@mit.edu> wrote:
This isn't an obscure old-fashioned thing. People really do use
this syntax.Given how little code now supports 10.1 meaning 10.0.0.1, that seems a
questionable point.
All code that uses gethostbyname() on, at least, Linux, Solaris,
Windows XP,
OS X and (I think) the BSDs and anything else that's even vaguely
posix uses it.
I don't think that's terribly relevant to the PG inet types, though.
Rejecting any
input format that's not a dotted-quad seems the safest thing to do,
and doesn't
lose any useful functionality. Given the number of people in this
thread who
think that the (non-standard, archaic) behaviour of bind is correct
it's clear that
accepting anything other than a real dotted-quad will lead to an
inconsistency
between what the data represents and what the user thinks it
represents, and
that's bound to cause problems.
Cheers,
Steve
"Larry Rosenman" <ler@lerctr.org> writes:
We had a **LONG** discussion on the I/O formats back in the 7.2 timeframe.
the current
behavior is the result of that.
Well I wasn't around for 7.2 but I was for a discussion around 7.3, maybe it's
the same one. Regardless, back then there was an implied assumption that any
change would affect both types. I couldn't convince people because 10.1/16 for
a network address range really is a reasonable abbreviation for 10.1.0.0.
It turns out network address ranges and hosts really don't have the same needs
at all. 10.1 expanding to 10.1.0.0 is really nonsensical for hosts and 10.1
expanding to 10.0.0.1 is nonsensical for network address ranges.
Now that they're going to be two different types proper I think it's not a bad
idea at all to revisit any design decisions that were made as a consequence of
them being mashed into one pseudo type.
Note that this would be entirely backwards compatible since inet doesn't
actually accept any abbreviated syntaxes at all currently. Only cidr does.
--
greg
