|From:||Andreas Karlsson <andreas(at)proxel(dot)se>|
|To:||emre(at)hasegeli(dot)com, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Cc:||Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>, Erik Rijkers <er(at)xs4all(dot)nl>, Emanuel Calvo <3manuek(at)esdebian(dot)org>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Nicolas Barbier <nicolas(dot)barbier(at)gmail(dot)com>, Claudio Freire <klaussfreire(at)gmail(dot)com>, Josh Berkus <josh(at)agliodbs(dot)com>, Andres Freund <andres(at)2ndquadrant(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>|
|Subject:||Re: BRIN range operator class|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
I made a quick review for your patch, but I would like to see someone
who was involved in the BRIN work comment on Emre's design issues. I
will try to answer them as best as I can below.
I think minimax indexes on range types seems very useful, and inet/cidr
too. I have no idea about geometric types. But we need to fix the issues
with empty ranges and IPv4/IPv6 for these indexes to be useful.
The current code compiles but the brin test suite fails.
I tested the indexes a bit and they seem to work fine, except for cases
where we know it to be broken like IPv4/IPv6.
The new code is generally clean and readable.
I think some things should be broken out in separate patches since they
are unrelated to this patch.
- The addition of &< and >& on inet types.
- The fix in brin_minmax.c.
Your brin tests seems to forget &< and >& for inet types.
The tests should preferably be extended to support ipv6 and empty ranges
once we have fixed support for those cases.
The /* If the it is all nulls, it cannot possibly be consistent. */
comment is different from the equivalent comment in brin_minmax.c. I do
not see why they should be different.
In brin_inclusion_union() the "if (col_b->bv_allnulls)" is done after
handling has_nulls, which is unlike what is done in brin_minmax_union(),
which code is right? I am leaning towards the code in
brin_inclusion_union() since you can have all_nulls without has_nulls.
On 12/14/2014 09:04 PM, Emre Hasegeli wrote:
>> To support more operators I needed to change amstrategies and
>> amsupport on the catalog. It would be nice if amsupport can be set
>> to 0 like am strategies.
> I think it would be nicer to get the functions from the operators
> with using the strategy numbers instead of adding them directly as
> support functions. I looked around a bit but couldn't find
> a sensible way to support it. Is it possible without adding them
> to the RelationData struct?
Yes that would be nice, but I do not think the current solution is terrible.
> This problem remains. There is also a similar problem with the
> range types, namely empty ranges. There should be special cases
> for them on some of the strategies. I tried to solve the problems
> in several different ways, but got a segfault one line or another.
> This makes me think that BRIN framework doesn't support to store
> different types than the indexed column in the values array.
> For example, brin_deform_tuple() iterates over the values array and
> copies them using the length of the attr on the index, not the length
> of the type defined by OpcInfo function. If storing another types
> aren't supported, why is it required to return oid's on the OpcInfo
> function. I am confused.
I leave this to someone more knowledgable about BRIN to answer.
> I didn't try to support other geometric types than box as I couldn't
> managed to store a different type on the values array, but it would
> be nice to get some feedback about the overall design. I was
> thinking to add a STORAGE parameter to the index to support other
> geometric types. I am not sure that adding the STORAGE parameter
> to be used by the opclass implementation is the right way. It
> wouldn't be the actual thing that is stored by the index, it will be
> an element in the values array. Maybe, data type specific opclasses
> is the way to go, not a generic one as I am trying.
I think a STORAGE parameter sounds like a good idea. Could it also be
used to solve the issue with IPv4/IPv6 by setting the storage type to
custom? Or is that the wrong way to fix things?
|Next Message||David Fetter||2015-01-11 00:41:16||Re: POLA violation with \c service=|
|Previous Message||Andres Freund||2015-01-11 00:22:01||Re: s_lock.h default definitions are rather confused|