Re: BRIN range operator class

> Yeah, there is still a test which fails in opr_sanity.

I attached an additional patch to remove extra pg_amproc entries from
minmax operator classes. It fixes the test as a side effect.

>> 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.

It is now split.

> = 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)

There is not much we can do about it. It looks like the problem in
here is the selectivity estimation.

> - Found a typo in the docs: "withing the range"

Fixed.

> - Why have you removed the USE_ASSERT_CHECKING code from brin.c?

Because it doesn't work with the new operator class. We don't set the
union field when there are elements that are not mergeable.

> - Remove redundant "or not" from "/* includes empty element or not */".

Fixed.

> - Minor grammar gripe: Change "Check that" to "Check if" in the comments in
> brin_inclusion_add_value().

Fixed.

> - Wont the code incorrectly return false if the first added element to an
> index page is empty?

No, column->bv_values[2] is set to true for the first empty element.

> - 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.

I changed it for all empty range checks.

> - 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"

All of them are fixed.

> - 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 fixed it on the new version. Tests wasn't failing because they were
using minimal operator class for quality.

> - 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.

Changed as you suggested.

> - Compact random spaces in "select numrange(1.0, 2.0) + numrange(2.5, 3.0); -- should fail"

There was a tab in there. Now it is replaced with a space.