Bug in create operator and/or initdb
The following seems to me a bug in either initdb or create operator:
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
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.
Obviously, it should either have a corresponding function declaration, or it should be possible to create the operators using a binary compatible function (eg: where a binary compatible cast exists).
I propose, that the create operator syntax be modified to accept:
PROCEDURE = function_name (type{,type})
and that checks be made for the existence of binary compatible casts between the two (four) types.
Kind Regards,
John
"John Hansen" <john@geeknet.com.au> 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
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
Import Notes
Resolved by subject fallback
"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
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
Import Notes
Resolved by subject fallback
"John Hansen" <john@geeknet.com.au> 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
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';
John Hansen <john@geeknet.com.au> 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
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
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 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
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
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/4which 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;
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:
Show quoted text
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/4which 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
Tom Lane <tgl@sss.pgh.pa.us> writes:
steve=# select '224.0.0.0'::cidr;
cidr
-------------
224.0.0.0/4which 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
Paul Vixie <paul@vix.com> 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
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.
Paul Vixie <paul@vix.com> 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