Cleaning up the INET/CIDR mess

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: Cleaning up the INET/CIDR mess
Date: 2006-01-24 18:23:17
Message-ID: 20481.1138126997@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Mark Kirkwood 2006-01-24 21:49:08 Re: [PATCHES] postmaster/postgres merge for testing
Previous Message Tom Lane 2006-01-24 17:00:47 Re: Weird pg_dumpall bug?