Thread: Re: [BUGS] Bug in create operator and/or initdb
> My opinion is that this is a very bogus shortcut in the > network datatype code. There are no cases outside the > inet/cidr group where an operator doesn't exactly match its > underlying function. (The whole business of inet and cidr > being almost but not quite the same type is maldesigned > anyway...) > > The right solution for you is to declare two SQL functions. > Whether you make them point at the same underlying C code is > up to you. Right,... In that case may I suggest fixing the catalog so network_* functions exists for both datatypes! Anything less I'd consider inconsistent... Kind regards, John
"John Hansen" <john@geeknet.com.au> writes:
> In that case may I suggest fixing the catalog so network_* functions exists for both datatypes!
Redesigning the inet/cidr distinction is on the to-do list (though I'm
afraid not very high on the list). ISTM it should either be one type
with a distinguishing bit in the runtime representation, or two types
with no such bit needed. Having both is a schizophrenic design. It's
led directly to bugs in the past, and I think there are still some
corner cases that act oddly (see the archives).
regards, tom lane
On Sat, Jan 29, 2005 at 10:07:30PM -0500, Tom Lane wrote: > "John Hansen" <john@geeknet.com.au> writes: > > In that case may I suggest fixing the catalog so network_* functions exists for both datatypes! > > Redesigning the inet/cidr distinction is on the to-do list (though I'm > afraid not very high on the list). ISTM it should either be one type > with a distinguishing bit in the runtime representation, or two types > with no such bit needed. Having both is a schizophrenic design. It's > led directly to bugs in the past, and I think there are still some > corner cases that act oddly (see the archives). From a network engineering point of view the inet type is utterly bogus. I'm not aware of data of that type being needed or used in any real application. Given that, the complexity that it causes simply by existing seems too high a cost. I suspect that the right thing to do is to kill the inet type entirely, and replace it with a special case of cidr. (And possibly then to kill cidr and replace it with something that can be indexed more effectively.) For a replacement type, how important is it that it be completely compatible with the existing inet/cidr types? Is anyone actually using inet types with a non-cidr mask? Cheers, Steve
Steve Atkins <steve@blighty.com> writes:
> For a replacement type, how important is it that it be completely
> compatible with the existing inet/cidr types? Is anyone actually using
> inet types with a non-cidr mask?
If you check the archives you'll discover that our current inet/cidr
types were largely designed and implemented by Paul Vixie (yes, that
Vixie). I'm disinclined to second-guess Paul about the external
definition of these types; I just want to rationalize the internal
representation a bit. In particular we've got some issues about
conversions between the two types ...
regards, tom lane
On Sun, 30 Jan 2005, Tom Lane wrote: > Steve Atkins <steve@blighty.com> writes: >> For a replacement type, how important is it that it be completely >> compatible with the existing inet/cidr types? Is anyone actually using >> inet types with a non-cidr mask? > > If you check the archives you'll discover that our current inet/cidr > types were largely designed and implemented by Paul Vixie (yes, that > Vixie). I'm disinclined to second-guess Paul about the external > definition of these types; I just want to rationalize the internal > representation a bit. In particular we've got some issues about > conversions between the two types ... Please do **NOT** break the external representations. We had enough fights about that 2-3 releases ago, and I personally don't want to revisit them. Yes, we do flakey things with inet on the masking stuff. LER -- Larry Rosenman http://www.lerctr.org/~ler Phone: +1 972-414-9812 E-Mail: ler@lerctr.org US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749
On Sun, Jan 30, 2005 at 09:49:43PM -0600, Larry Rosenman wrote: > On Sun, 30 Jan 2005, Tom Lane wrote: > > >Steve Atkins <steve@blighty.com> writes: > >>For a replacement type, how important is it that it be completely > >>compatible with the existing inet/cidr types? Is anyone actually using > >>inet types with a non-cidr mask? > > > >If you check the archives you'll discover that our current inet/cidr > >types were largely designed and implemented by Paul Vixie (yes, that > >Vixie). I'm disinclined to second-guess Paul about the external > >definition of these types; I just want to rationalize the internal > >representation a bit. In particular we've got some issues about > >conversions between the two types ... > > Please do **NOT** break the external representations. We had enough fights > about that 2-3 releases ago, and I personally don't want to revisit them. > Yes, we do flakey things with inet on the masking stuff. Well, if you want the ability to store both a host address and a netmask in the same datatype the inet masking stuff makes sense. That's not really a useful datatype for any actual use, but it's fairly well-defined. The problem is that when someone looks at the docs they'll see inet as the obvious datatype to use to store IP addresses, and it isn't very good for that. But that's not all that's flakey, unfortunately. The CIDR input format is documented to be classful, which in itself is horribly obsolete and completely useless in this decades internet (and was when the current code was written in '98). But the implementation isn't either classful or classless, and the behaviour disagrees with documented behaviour, and the behaviour you'd reasonably expect, in many cases. -- Class A - documented to be 10.0.0.0/8 steve=# select '10.0.0.0'::cidr; cidr ------------- 10.0.0.0/32 -- Class B - documented to be 128.0.0.0/16 steve=# select '128.0.0.0'::cidr; cidr -------------- 128.0.0.0/32 -- Class C - documented to be 223.10.0.0/24 steve=# select '223.10.0.0'::cidr; cidr --------------- 223.10.0.0/32 -- Class D steve=# select '224.10.0.0'::cidr; ERROR: invalid cidr value: "224.10.0.0" DETAIL: Value has bits set to rightof mask. steve=# select '224.0.0.0'::cidr; cidr ------------- 224.0.0.0/4 -- Class E steve=# select '240.10.0.0'::cidr; cidr --------------- 240.10.0.0/32 I use postgresql for network-related applications and for IP address related data mining, so I'm dealing with IP addresses in postgresql on a daily basis. The cidr type, including it's external interface, is simply broken. There is no way to fix it that doesn't change that external interface. I know of at least two independant implementations of function IP address types that have been put together for specific projects to implement a working IP datatype. The ability to use gist indexes on them to accelerate range-based lookups is a bonus. If it's not possible (for backwards compatibility reasons) to fix inet+cidr, would migrating them out to contrib be a possibility? Data types in the core tend to be widely used, even if they're broken and there are better datatypes implemented as external modules. Cheers, Steve
Steve Atkins <steve@blighty.com> writes:
> The cidr type, including it's external interface, is simply broken.
That is a large claim that I don't think you have demonstrated.
The only one of your examples that seems to me to contradict the
documentation is this one:
steve=# select '224.0.0.0'::cidr; cidr ------------- 224.0.0.0/4
which should be /32 according to what the docs say:
: If y is omitted, it is calculated using assumptions from the older
: classful network numbering system, except that it will be at least large
: enough to include all of the octets written in the input.
The bogus netmask is in turn responsible for this case:
steve=# select '224.10.0.0'::cidr; ERROR: invalid cidr value: "224.10.0.0" DETAIL: Value has bits set to right of
mask.
Looking at the source code, there seems to be a special case for "class D"
network numbers that causes the code not to extend y to cover the
supplied inputs:
/* If no CIDR spec was given, infer width from net class. */ if (bits == -1) { if (*odst >= 240) /*
ClassE */ bits = 32; else if (*odst >= 224) /* Class D */ bits = 4; 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 >= 8 && bits < ((dst - odst) * 8)) ^^^^^^^^^ bits = (dst - odst) * 8; }
I think the test for "bits >= 8" should be removed. Does anyone know
why it's there?
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > steve=# select '224.0.0.0'::cidr; > cidr > ------------- > 224.0.0.0/4 > > which should be /32 according to what the docs say: 224-239 are multicast addresses. Making it /4 makes the entire multicast address space one network block which is about as reasonable an answer as anything else. > if (bits >= 8 && bits < ((dst - odst) * 8)) > ^^^^^^^^^ > bits = (dst - odst) * 8; > } > > I think the test for "bits >= 8" should be removed. Does anyone know > why it's there? I guess Vixie figured network blocks subdividing multicast address space weren't a sensible concept? It's a bit of a strange constraint to hard code into the C code though. Incidentally, how can that code possibly work? It treats odst as a pointer in some places but then calculates bits using arithmetic on it directly without dereferencing? -- greg
On Mon, Jan 31, 2005 at 12:16:26PM -0500, Tom Lane wrote: > Steve Atkins <steve@blighty.com> writes: > > The cidr type, including it's external interface, is simply broken. > > That is a large claim that I don't think you have demonstrated. > The only one of your examples that seems to me to contradict the > documentation is this one: > > steve=# select '224.0.0.0'::cidr; > cidr > ------------- > 224.0.0.0/4 > > which should be /32 according to what the docs say: OK. If this sort of thing is considered a bug, rather than part of the external interface that shouldn't be changed, then I'd agree that cidr isn't entirely broken and it may well be possible to improve it without changing the interface. /me goes grovelling through the IPv6 inet code... Cheers, Steve
when my cidr datatype was integrated into pgsql, the decision was made to
incorporate a copy of bind's inet_net_pton.c rather than add a link-time
dependence to libbind.a (libbind.so). thus, when this bug was fixed in
2003:
----------------------------
revision 1.14
date: 2003/08/20 02:21:08; author: marka; state: Exp; lines: +10 -4
1580. [bug] inet_net_pton() didn't fully handle implicit multicast IPv4 network
addresses.
the pgsql "fork" of this code did not benefit from the fix. the patch was:
Index: inet_net_pton.c
===================================================================
RCS file: /proj/cvs/prod/bind8/src/lib/inet/inet_net_pton.c,v
retrieving revision 1.13
retrieving revision 1.14
diff -u -r1.13 -r1.14
--- inet_net_pton.c 27 Sep 2001 15:08:38 -0000 1.13
+++ inet_net_pton.c 20 Aug 2003 02:21:08 -0000 1.14
@@ -16,7 +16,7 @@ */#if defined(LIBC_SCCS) && !defined(lint)
-static const char rcsid[] = "$Id: inet_net_pton.c,v 1.13 2001/09/27 15:08:38 marka Exp $";
+static const char rcsid[] = "$Id: inet_net_pton.c,v 1.14 2003/08/20 02:21:08 marka Exp $";#endif#include
"port_before.h"
@@ -59,7 +59,7 @@ * Paul Vixie (ISC), June 1996 */static int
-inet_net_pton_ipv4( const char *src, u_char *dst, size_t size) {
+inet_net_pton_ipv4(const char *src, u_char *dst, size_t size) { static const char xdigits[] =
"0123456789abcdef"; static const char digits[] = "0123456789"; int n, ch, tmp = 0, dirty, bits;
@@ -152,7 +152,7 @@ if (*odst >= 240) /* Class E */ bits = 32;
elseif (*odst >= 224) /* Class D */
- bits = 4;
+ bits = 8; else if (*odst >= 192) /* Class C */ bits = 24;
else if (*odst >= 128) /* Class B */
@@ -160,8 +160,14 @@ else /* Class A */ bits = 8;
/*If imputed mask is narrower than specified octets, widen. */
- if (bits >= 8 && bits < ((dst - odst) * 8))
+ if (bits < ((dst - odst) * 8)) bits = (dst - odst) * 8;
+ /*
+ * If there are no additional bits specified for a class D
+ * address adjust bits to 4.
+ */
+ if (bits == 8 && *odst == 224)
+ bits = 4; } /* Extend network to cover the actual mask. */ while (bits >
((dst- odst) * 8)) {
re:
> To: Steve Atkins <steve@blighty.com>
> Cc: pgsql-hackers <pgsql-hackers@postgresql.org>, paul@vix.com
> Subject: Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
> Comments: In-reply-to Steve Atkins <steve@blighty.com>
> message dated "Mon, 31 Jan 2005 07:23:05 -0800"
> Date: Mon, 31 Jan 2005 12:16:26 -0500
> From: Tom Lane <tgl@sss.pgh.pa.us>
>
> Steve Atkins <steve@blighty.com> writes:
> > The cidr type, including it's external interface, is simply broken.
>
> That is a large claim that I don't think you have demonstrated.
> The only one of your examples that seems to me to contradict the
> documentation is this one:
>
> steve=# select '224.0.0.0'::cidr;
> cidr
> -------------
> 224.0.0.0/4
>
> which should be /32 according to what the docs say:
>
> : If y is omitted, it is calculated using assumptions from the older
> : classful network numbering system, except that it will be at least large
> : enough to include all of the octets written in the input.
>
> The bogus netmask is in turn responsible for this case:
>
> steve=# select '224.10.0.0'::cidr;
> ERROR: invalid cidr value: "224.10.0.0"
> DETAIL: Value has bits set to right of mask.
>
>
> Looking at the source code, there seems to be a special case for "class D"
> network numbers that causes the code not to extend y to cover the
> supplied inputs:
>
> /* If no CIDR spec was given, infer width from net class. */
> if (bits == -1)
> {
> if (*odst >= 240) /* Class E */
> bits = 32;
> else if (*odst >= 224) /* Class D */
> bits = 4;
> 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 >= 8 && bits < ((dst - odst) * 8))
> ^^^^^^^^^
> bits = (dst - odst) * 8;
> }
>
> I think the test for "bits >= 8" should be removed. Does anyone know
> why it's there?
>
> regards, tom lane