On 02/11/2015 07:34 PM, Emre Hasegeli wrote:
The current code compiles but the brin test suite fails.
Now, only a test in .
Yeah, there is still a test which fails in opr_sanity.
Yes but they were also required by this patch. This version adds more
functions and operators. I can split them appropriately after your
review.
Ok, sounds fine to me.
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.
Looks good as far as I can tell.
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.
Looks to me like this should work.
= New comments
- Searching for the empty range is slow since the empty range matches
all brin ranges.
EXPLAIN ANALYZE SELECT * FROM foo WHERE r = '[1,1)';
QUERY PLAN
-----------------------------------------------------------------------------------------------------------------------
Bitmap Heap Scan on foo (cost=12.01..16.02 rows=1 width=14) (actual
time=47.603..47.605 rows=1 loops=1)
Recheck Cond: (r = 'empty'::int4range)
Rows Removed by Index Recheck: 200000
Heap Blocks: lossy=1082
-> Bitmap Index Scan on foo_r_idx (cost=0.00..12.01 rows=1
width=0) (actual time=0.169..0.169 rows=11000 loops=1)
Index Cond: (r = 'empty'::int4range)
Planning time: 0.062 ms
Execution time: 47.647 ms
(8 rows)
- Found a typo in the docs: "withing the range"
- Why have you removed the USE_ASSERT_CHECKING code from brin.c?
- Remove redundant "or not" from "/* includes empty element or not */".
- Minor grammar gripe: Change "Check that" to "Check if" in the comments
in brin_inclusion_add_value().
- Wont the code incorrectly return false if the first added element to
an index page is empty?
- Would it be worth optimizing the code by checking for empty ranges
after checking for overlap in brin_inclusion_add_value()? I would
imagine that empty ranges are rare in most use cases.
- Typo in comment: "If the it" -> "If it"
- Typo in comment: "Note that this strategies" -> "Note that these
strategies"
- Typo in comment: "inequality strategies does not" -> "inequality
strategies do not"
- Typo in comment: "geometric types which uses" -> "geometric types
which use"
- I get 'ERROR: missing strategy 7 for attribute 1 of index
"bar_i_idx"' when running the query below. Why does this not fail in the
test suite? The overlap operator works just fine. If I read your code
correctly other strategies are also missing.
SELECT * FROM bar WHERE i = '::1';
- I do not think this comment is true "Used to determine the addresses
have a common union or not". It actually checks if we can create range
which contains both ranges.
- Compact random spaces in "select numrange(1.0, 2.0) + numrange(2.5,
3.0); -- should fail"
--
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
