|From:||Andreas Karlsson <andreas(at)proxel(dot)se>|
|Cc:||Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, 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|
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
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
> 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)';
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
- 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
- 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
- Typo in comment: "inequality strategies does not" -> "inequality
strategies do not"
- Typo in comment: "geometric types which uses" -> "geometric types
- 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"
|Next Message||Michael Paquier||2015-03-08 11:19:39||Re: In-core regression tests for replication, cascading, archiving, PITR, etc.|
|Previous Message||Fabrízio de Royes Mello||2015-03-08 03:29:16||Re: Strange debug message of walreciver?|