Re: BRIN range operator class

Hi,

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.

= Review

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?

-- 
Andreas Karlsson