Re: BRIN range operator class

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

In response to

Responses

Browse pgsql-hackers by date

  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?