From: | Emre Hasegeli <emre(at)hasegeli(dot)com> |
---|---|
To: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> |
Cc: | Andreas Karlsson <andreas(at)proxel(dot)se>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Erik Rijkers <er(at)xs4all(dot)nl>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Nicolas Barbier <nicolas(dot)barbier(at)gmail(dot)com>, Claudio Freire <klaussfreire(at)gmail(dot)com>, Josh Berkus <josh(at)agliodbs(dot)com> |
Subject: | Re: BRIN range operator class |
Date: | 2015-04-14 14:45:32 |
Message-ID: | CAE2gYzxQ-Gk3q3jYWT=1eNLEbSgCgU28+1axML4oMCwjBkPuqw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
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.
From | Date | Subject | |
---|---|---|---|
Next Message | Heikki Linnakangas | 2015-04-14 14:50:23 | Re: FPW compression leaks information |
Previous Message | Heikki Linnakangas | 2015-04-14 14:10:59 | Re: What exactly is our CRC algorithm? |