Re: amcheck support for BRIN indexes

Hi,

I would like share some thoughts about 'heap all consistent' part and
one of the open questions:

The idea behind 'heap all consistent' is to use heap data to validate
the index. BRIN doesn't store heap tuples, so there is no
straightforward way to check if every tuple was indexed or not. We
have range data, so we need to do something with every heap tuple and
corresponding range. Something that will tell us if the range data
covers the heap tuple or not. What options I see here:

1) We can use the addValue() function. It returns FALSE if range data
was not changed (in other words range data already covers heap tuple
data that we pass to the function). It's very easy to do, we can use
heap tuples directly. But the problem I see here is that addValue()
can return TRUE even if heap tuple data have been already covered by
the range, but range data itself changed for some reason (some new
algorithm were applied for instance). So I think we can have some
false positives that we can do nothing about.

2) We can use the consistent() function. It requires ScanKey and
returns true if the range data satisfies ScanKey's condition. So we
need to convert every heap tuple into ScanKey somehow. This approach
is implemented now in the patch, so I tried to describe all details
about heap tuple to ScanKey conversion in the comment:

/*
* Prepare ScanKey for index attribute.
*
* ConsistentFn requires ScanKey, so we need to generate ScanKey for every
* attribute somehow. We want ScanKey that would result in TRUE for every heap
* tuple within the range when we use its indexed value as sk_argument.
* To generate such a ScanKey we need to define the right operand type
and the strategy number.
* Right operand type is a type of data that index is built on, so
it's 'opcintype'.
* There is no strategy number that we can always use,
* because every opclass defines its own set of operators it supports
and strategy number
* for the same operator can differ from opclass to opclass.
* So to get strategy number we look up an operator that gives us
desired behavior
* and which both operand types are 'opcintype' and then retrieve the
strategy number for it.
* Most of the time we can use '='. We let user define operator name
in case opclass doesn't
* support '=' operator. Also, if such operator doesn't exist, we
can't proceed with the check.
*
* Generated once, and will be reused for all heap tuples.
* Argument field will be filled for every heap tuple before
* consistent function invocation, so leave it NULL for a while.
*
*/

With this approach function brin_check() has optional parameter
'consistent_operator_names' that it seems to me could be very
confusing for users. In general I think this is the most complex
approach both in terms of use and implementation.

3) The approach that seems to me the most clear and straightforward:
to add new optional function to BRIN opclass API. The function that
would say if passed value is covered with the current range data. it's
exactly what we want to know, so we can use heap data directly here.
Something like that:

bool withinRange(BrinDesc *bdesc, BrinValues *column, Datum val, bool isnull)

It could be an optional function that will be implemented for all core
BRIN opclasses. So if somebody wants to use it for some custom opclass
they will need to implement it too, but it's not required. I
understand that adding something to the index opclass API requires
very strong arguments. So the argument here is that it will let to do
brin check very robust (without possible false positives as in the
first approach) and easy to use (no additional parameters in the check
function). Also, the withinRange() function could be written in such a
way that it would be more efficient for our task than addValue() or
consistent().

I'd be glad to hear your thoughts!

Best regards,
Arseniy Mukhin