Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
Bruce Momjian pgman@candle.pha.pa.us writes: Added to release checklist: * Update inet/cidr data types with newest Bind patches You should also add check for zic database updates. regards, tom lane ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
Tom Lane wrote: Paul Vixie [EMAIL PROTECTED] writes: i have two suggestions. first, look at the rest of the current source file, in case there are other fixes. Right, I already grabbed the latest. second, track changes this source file during your release engineering process for each new pgsql version. Bruce, do you think this is worth adding to RELEASE_CHANGES? inet_net_ntop.c and inet_net_pton.c are both extracted from the BIND distribution. But they're hardly the only files we took from elsewhere. Yes, I do. Most of the stuff we pull from other OS projects has clearly-defined behavior, while inet/cidr seem to be still in flux a little. Added to release checklist: * Update inet/cidr data types with newest Bind patches -- 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 19073 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
Tom Lane wrote: Bruce Momjian pgman@candle.pha.pa.us writes: Added to release checklist: * Update inet/cidr data types with newest Bind patches You should also add check for zic database updates. Uh, we already have: * Update timezone data to match latest zic database (see src/timezone/README) -- 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 19073 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
On Sun, 30 Jan 2005, Tom Lane wrote: Steve Atkins [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
On Sun, Jan 30, 2005 at 09:49:43PM -0600, Larry Rosenman wrote: On Sun, 30 Jan 2005, Tom Lane wrote: Steve Atkins [EMAIL PROTECTED] 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 right of 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 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
Steve Atkins [EMAIL PROTECTED] 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
---(end of broadcast)---
TIP 5: Have you checked our extensive FAQ?
http://www.postgresql.org/docs/faq
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
Paul Vixie [EMAIL PROTECTED] writes: 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). We didn't really want to assume that all platforms are using libbind :-( 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: Ah-hah. Many thanks for supplying the patch --- will integrate it. regards, tom lane ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
Tom Lane [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
On Mon, Jan 31, 2005 at 12:16:26PM -0500, Tom Lane wrote: Steve Atkins [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
Paul Vixie [EMAIL PROTECTED] writes: i have two suggestions. first, look at the rest of the current source file, in case there are other fixes. Right, I already grabbed the latest. second, track changes this source file during your release engineering process for each new pgsql version. Bruce, do you think this is worth adding to RELEASE_CHANGES? inet_net_ntop.c and inet_net_pton.c are both extracted from the BIND distribution. But they're hardly the only files we took from elsewhere. regards, tom lane ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
We didn't really want to assume that all platforms are using libbind :-( i think you could have, at the time, since windows wasn't even a gleam in pgsql's eye. even now, libbind would be a dependable universal dependency, since we publish windows binaries. the pgsql fork of this code did not benefit from the fix. the patch was: Ah-hah. Many thanks for supplying the patch --- will integrate it. i have two suggestions. first, look at the rest of the current source file, in case there are other fixes. second, track changes this source file during your release engineering process for each new pgsql version. ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
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 - 1.13
+++ inet_net_pton.c 20 Aug 2003 02:21:08 - 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;
else if (*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 [EMAIL PROTECTED]
Cc: pgsql-hackers pgsql-hackers@postgresql.org, [EMAIL PROTECTED]
Subject: Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
Comments: In-reply-to Steve Atkins [EMAIL PROTECTED]
message dated Mon, 31 Jan 2005 07:23:05 -0800
Date: Mon, 31 Jan 2005 12:16:26 -0500
From: Tom Lane [EMAIL PROTECTED]
Steve Atkins [EMAIL PROTECTED] 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
---(end of broadcast)---
TIP 5: Have you checked our extensive FAQ?
http://www.postgresql.org/docs/faq
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
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.) Yes, which is actually what brought this to my attention. I'll be sending an rtree index implementation shortly for review/comments. 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? I wouldn't think so, anyone I've spoken with has come up with other ways of managing that kind of info, because of, as you mentioned, it's lack of proper index methods. Kind Regards, John Hansen ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
John Hansen [EMAIL PROTECTED] writes: I wouldn't think so, anyone I've spoken with has come up with other ways of managing that kind of info, because of, as you mentioned, it's lack of proper index methods. On the contrary I'm using it for something that isn't really what it was designed for precisely *because* of the index methods. What index access methods are you looking for that are lacking? db= explain select * from foo where foo_code '4.0.0.0/8'; QUERY PLAN -- Index Scan using foo_foo_code on foo (cost=0.00..34.56 rows=1695 width=229) Index Cond: ((foo_code '4.0.0.0/8'::cidr) AND (foo_code = '4.255.255.255'::cidr)) Filter: (foo_code '4.0.0.0/8'::cidr) (3 rows) -- greg ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
On the contrary I'm using it for something that isn't really what it was designed for precisely *because* of the index methods. What index access methods are you looking for that are lacking? db= explain select * from foo where foo_code '4.0.0.0/8'; explain select * from foo where foo_code '220.244.179.214/32'; ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
John Hansen [EMAIL PROTECTED] writes: On the contrary I'm using it for something that isn't really what it was designed for precisely *because* of the index methods. What index access methods are you looking for that are lacking? db= explain select * from foo where foo_code '4.0.0.0/8'; explain select * from foo where foo_code '220.244.179.214/32'; Note also that the btree optimization for depends on having a plan-time-constant righthand side; so it's useless for joins, for instance. I didn't look closely, but I'd suppose that rtree could help for searches without that constraint. Looking to the future, it might be better to base this on gist instead of rtree indexes --- gist is being worked on semi-actively, rtree isn't really being touched at all. But the immediate problem is that I don't think we can integrate this if it doesn't handle IPv6 addresses. We aren't going to want to backslide on having full IPv6 support. regards, tom lane ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
Note also that the btree optimization for depends on having a plan-time-constant righthand side; so it's useless for joins, for instance. I didn't look closely, but I'd suppose that rtree could help for searches without that constraint. Indeed it can... tho rtree seems to be unable to do merge joins, so you only get an index scan on one of the joined tables. Which index is chosen has proven a bit hard to predict. Looking to the future, it might be better to base this on gist instead of rtree indexes --- gist is being worked on semi-actively, rtree isn't really being touched at all. Yea, rtree is also broken, tho I think andrew is going to attempt fixing it. But the immediate problem is that I don't think we can integrate this if it doesn't handle IPv6 addresses. We aren't going to want to backslide on having full IPv6 support. Right,. i'll be adding ipv6 support... ... John ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [HACKERS] [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 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
John Hansen [EMAIL PROTECTED] writes: CREATE FUNCTION my_func (inet, inet) as '$libdir/my_func.so' LANGUAGE 'C' IMMUTABLE STRICT; CREATE OPERATOR ( PROCEDURE = my_func, LEFTARG = cidr, RIGHTARG = cidr, RESTRICT = contsel, JOIN = contjoinsel ); ERROR: function my_func(cidr, cidr) does not exist Right ... Now, if you look at the catalog, and the (less than operator) as an example you will see that: Two operators are defined for - one for inet,inet and another for cidr,cidr. Only one function exists named network_lt, and is declared as taking (inet,inet) as arguments. 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. regards, tom lane ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
John Hansen [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
On Sat, Jan 29, 2005 at 10:07:30PM -0500, Tom Lane wrote: John Hansen [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [HACKERS] [BUGS] Bug in create operator and/or initdb
Steve Atkins [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
