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.



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.


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 ''::inet -|- ''::inet;
(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?


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.



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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to