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