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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
