From: | Andreas Karlsson <andreas(at)proxel(dot)se> |
---|---|
To: | emre(at)hasegeli(dot)com |
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 |
Date: | 2015-03-08 04:44:36 |
Message-ID: | 54FBD3B4.3040202@proxel.se |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
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? |