Re: GiST support for inet datatypes

Hi,

I will review your two patches (gist support + selectivity). This is 
part 1 of my review. I will look more into the actual GiST 
implementation in a couple of days, but thought I could provide you with 
my initial input right away.

inet-gist
---------

General:

I like the idea of the patch and think the && operator is useful for 
exclusion constraints, and that indexing the contains operator is useful 
for IP-address lookups. There is an extension, ip4r, which adds a GiST 
indexed type for IP ranges but since we have the inet type in core I 
think it should have GiST indexes.

I am not convinced an adjacent operator is useful for the inet type, but 
if it is included it should be indexed just like -|- of ranges. We 
should try to keep these lists of indexed operators the same.

Compilation:

Compiled without errors.

Regression tests:

One of the broken regression tests seems unrelated to the selectivity 
functions.

-- Make a list of all the distinct operator names being used in particular
-- strategy slots.

I think it would be fine just to add the newly indexed operators here, 
but the more indexed operators we get in the core the less useful this 
test becomes.

Source code:

The is adjacent to operator, -|-, does not seem to be doing the right 
thing. In the code it is implemented as the inverse of && which is not 
true. The below comparison should return false.

SELECT '10.0.0.1'::inet -|- '127.0.0.1'::inet; ?column?
---------- t
(1 row)

I am a bit suspicious about your memcmp based optimization in 
bitncommon, but it could be good. Have you benchmarked it compared to 
doing the same thing with a loop?

Documentation:

Please use examples in the operator table which will evaluate to true, 
and for the && case an example where not both sides are the same.

I have not found a place either in the documentation where it is 
documented which operators are documented. I would suggest just adding a 
short note after the operators table.

inet-selfuncs
-------------

Compilation:

There were some new warnings caused by this patch (with gcc 4.8.2). The 
warnings were all about the use of uninitialized variables (right, 
right_min_bits, right_order) in inet_hist_overlap_selectivity. Looking 
at the code I see that they are harmless but you should still rewrite 
the loop to silence the warnings.

Regression tests:

There are two broken

-- Insist that all built-in pg_proc entries have descriptions

Here you should just add descriptions for the new functions in pg_proc.h.

-- Check that all opclass search operators have selectivity estimators.

Fails due to missing selectivity functions for the operators. I think 
you should at least for now do like the range types and use areajoinsel, 
contjoinsel, etc for the join selectivity estimation.

Source code:

The selectivity estimation functions, network_overlap_selectivity and 
network_adjacent_selectivity, should follow the naming conventions of 
the other selectivity estimation functions. They are named things like 
tsmatchsel, tsmatchjoinsel, and rangesel.

-- 
Andreas Karlsson