Re: BRIN range operator class

Emre Hasegeli <emre@hasegeli.com>

From: Emre Hasegeli <emre@hasegeli.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Andreas Karlsson <andreas@proxel.se>, Pg Hackers <pgsql-hackers@postgresql.org>, Erik Rijkers <er@xs4all.nl>, Simon Riggs <simon@2ndquadrant.com>, Robert Haas <robertmhaas@gmail.com>, Nicolas Barbier <nicolas.barbier@gmail.com>, Claudio Freire <klaussfreire@gmail.com>, Josh Berkus <josh@agliodbs.com>
Date: 2015-04-14T14:45:32Z
Lists: pgsql-hackers
> Judging from a quick look, I think patches 1 and 5 can be committed
> quickly; they imply no changes to other parts of BRIN.  (Not sure why 1
> and 5 are separate.  Any reason for this?)  Also patch 2.

Not much reason except that 1 includes only functions, but 5 includes operators.

> Patch 4 looks like a simple bugfix (or maybe a generalization) of BRIN
> framework code; should also be committable right away.  Needs a closer
> look of course.
>
> Patch 3 is a problem.  That code is there because the union proc is only
> used in a corner case in Minmax, so if we remove it, user-written Union
> procs are very likely to remain buggy for long.  If you have a better
> idea to test Union in Minmax, or some other way to turn that stuff off
> for the range stuff, I'm all ears.  Just lets make sure the support
> procs are tested to avoid stupid bugs.  Before I introduced that, my
> Minmax Union proc was all wrong.

I removed this test because I don't see a way to support it.  I
believe any other implementation that is more complicated than minmax
will fail in there.  It is better to cache them with the regression
tests, so I tried to improve them.  GiST, SP-GiST and GIN don't have
similar checks, but they have more complicated user defined functions.

> Patch 7 I don't understand.  Will have to look closer.  Are you saying
> Minmax will depend on Btree opclasses?  I remember thinking in doing it
> that way at some point, but wasn't convinced for some reason.

No, there isn't any additional dependency.  It makes minmax operator
classes use the procedures from the pg_amop instead of adding them to
pg_amproc.

It also makes the operator class safer for cross data type usage.
Actually, I just checked and find out that we got wrong answers from
index on the current master without this patch.  You can reproduce it
with this query on the regression database:

select * from brintest where timestampcol = '1979-01-29 11:05:09'::timestamptz;

inclusion-opclasses patch make it possible to add cross type brin
regression tests.  I will add more of them on the next version.

> Patch 6 seems the real meat of your own stuff.  I think there should be
> a patch 8 also but it's not attached ... ??

I had another commit not to intended to be sent.  Sorry about that.


Commits

  1. Refactor per-page logic common to all redo routines to a new function.

  2. Reduce use of heavyweight locking inside hash AM.

  3. Scan the buffer pool just once, not once per fork, during relation drop.

  4. Major patch from Thomas Lockhart <Thomas.G.Lockhart@jpl.nasa.gov>