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
-
Refactor per-page logic common to all redo routines to a new function.
- f8f4227976a2 9.5.0 cited
-
Reduce use of heavyweight locking inside hash AM.
- 76837c1507cb 9.3.0 cited
-
Scan the buffer pool just once, not once per fork, during relation drop.
- ece01aae4792 9.2.0 cited
-
Major patch from Thomas Lockhart <Thomas.G.Lockhart@jpl.nasa.gov>
- 9e2a87b62db8 7.1.1 cited