Re: BRIN range operator class

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