Re: BRIN range operator class - Mailing list pgsql-hackers

Thank you for looking at my patch again.  New version is attached
with a lot of changes and point data type support.

> 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.

Both of the cases are fixed on the new version.

> The current code compiles but the brin test suite fails.

Now, only a test in .

> 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.

Yes but they were also required by this patch.  This version adds more
functions and operators.  I can split them appropriately after your
review.

> - The addition of &< and >& on inet types.

I haven't actually added the operators, just the underlying procedures
for them to support basic comparison operators with the BRIN opclass.
I left them them out on the new version because of its new design.
We can add the operators later with documentation, tests and index
support.

> - The fix in brin_minmax.c.

It is already committed by Alvaro Herrera.  I can send another patch
to use pg_amop instead of pg_amproc on brin_minmax.c, if it is
acceptable.

> The tests should preferably be extended to support ipv6 and empty ranges
> once we have fixed support for those cases.

Done.

> 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.

Not to confuse with the empty ranges.  Also, there it supports other
types than ranges, like box.

> 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.

>> 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.

The new version does it this way.  It was required to support
strategies between different types.

>> 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 think I have fixed them.

>> 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?

I have fixed different addressed families by adding another support
function.

I used STORAGE parameter to support the point data type.  To make it
work I added some operators between box and point data type.  We can
support all geometric types with this method.