Re: BRIN indexes - TRAP: BadArgument

On 09/23/2014 10:04 PM, Alvaro Herrera wrote:
> + <para>
> + The <acronym>BRIN</acronym> implementation in <productname>PostgreSQL</productname>
> + is primarily maintained by &Aacute;lvaro Herrera.
> + </para>

We don't usually have such verbiage in the docs. The GIN and GiST pages
do, but I think those are a historic exceptions, not something we want
to do going forward.

> + <variablelist>
> + <varlistentry>
> + <term><function>BrinOpcInfo *opcInfo(void)</></term>
> + <listitem>
> + <para>
> + Returns internal information about the indexed columns' summary data.
> + </para>
> + </listitem>
> + </varlistentry>

I think you should explain what that internal information is. The
minmax-19a.patch adds the type OID argument to this; remember to update
the docs.

In SP-GiST, the similar function is called "config". It might be good to
use the same name here, for consistency across indexams (although I
actually like the "opcInfo" name better than "config")

The docs for the other support functions need to be updated, now that
you changed the arguments from DeformedBrTuple to BrinValues.

> + <!-- this needs improvement ... -->
> + To implement these methods in a generic ways, normally the opclass
> + defines its own internal support functions. For instance, minmax
> + opclasses add the support functions for the four inequality operators
> + for the datatype.
> + Additionally, the operator class must supply appropriate
> + operator entries,
> + to enable the optimizer to use the index when those operators are
> + used in queries.

The above needs improvement ;-)

> + BRIN indexes (a shorthand for Block Range indexes)
> + store summaries about the values stored in consecutive table physical block ranges.

"consecutive table physical block ranges" is quite a mouthful.

> + For datatypes that have a linear sort order, the indexed data
> + corresponds to the minimum and maximum values of the
> + values in the column for each block range,
> + which support indexed queries using these operators:
> +
> + <simplelist>
> + <member><literal>&lt;</literal></member>
> + <member><literal>&lt;=</literal></member>
> + <member><literal>=</literal></member>
> + <member><literal>&gt;=</literal></member>
> + <member><literal>&gt;</literal></member>
> + </simplelist>

That's the built-in minmax indexing strategy, yes, but you could have
others, even for datatypes with a linear sort order.

> + To find out the index tuple for a particular page range, we have an internal

s/find out/find/

> + new heap tuple contains null values but the index tuple indicate there are no

s/indicate/indicates/

> + Open questions
> + --------------
> +
> + * Same-size page ranges?
> + Current related literature seems to consider that each "index entry" in a
> + BRIN index must cover the same number of pages. There doesn't seem to be a

What is the related literature? Is there an academic paper or something
that should be cited as a reference for BRIN?

> + * TODO
> + * * ScalarArrayOpExpr (amsearcharray -> SK_SEARCHARRAY)
> + * * add support for unlogged indexes
> + * * ditto expressional indexes

We don't have unlogged indexes in general, so no need to list that here.
What would be needed to implement ScalarArrayOpExprs?

I didn't realize that expression indexes are still not supported. And I
see that partial indexes are not supported either. Why not? I wouldn't
expect BRIN to need to care about those things in particular; the
expressions for an expressional or partial index are handled in the
executor, no?

> + /*
> + * A tuple in the heap is being inserted. To keep a brin index up to date,
> + * we need to obtain the relevant index tuple, compare its stored values with
> + * those of the new tuple; if the tuple values are consistent with the summary
> + * tuple, there's nothing to do; otherwise we need to update the index.

s/compare/and compare/. Perhaps replace one of the semicolons with a
full stop.

> + * If the range is not currently summarized (i.e. the revmap returns InvalidTid
> + * for it), there's nothing to do either.
> + */
> + Datum
> + brininsert(PG_FUNCTION_ARGS)

There is no InvalidTid, as a constant or a #define. Perhaps replace with
"invalid item pointer".

> + /*
> + * XXX We need to know the size of the table so that we know how long to
> + * iterate on the revmap. There's room for improvement here, in that we
> + * could have the revmap tell us when to stop iterating.
> + */

The revmap doesn't know how large the table is. Remember that you have
to return all blocks that are not in the revmap, so you can't just stop
when you reach the end of the revmap. I think the current design is fine.

I have to stop now to do some other stuff. Overall, this is in pretty
good shape. In addition to little cleanup of things I listed above, and
similar stuff elsewhere that I didn't read through right now, there are
a few medium-sized items I'd still like to see addressed before you
commit this:

* expressional/partial index support
* the difficulty of testing the union support function that we discussed
earlier
* clarify the memory context stuff of support functions that we also
discussed earlier

- Heikki