On Sat, 2011-07-02 at 18:38 -0400, Tom Lane wrote:
> Jeff Davis <pgsql@j-davis.com> writes:
> > On Sat, 2011-06-18 at 13:20 -0700, Jeff Davis wrote:
> >> Interesting problem... the bug is in get_op_btree_interpretation() which
> >> has code like this:
> >> ...
> >> However, that's a bogus test, because btree_gist puts <> into an
> >> opfamily. Thus, catlist->n_members == 1 even though we really do need to
> >> look for the negator. Really, we need to unconditionally search for the
> >> operator as well as unconditionally searching for the negator.
>
> > Patch attached.
>
> I looked at this a bit. The proposed patch is inadequate, aside from
> any excess lookups it introduces, because there is similar logic in
> predtest.c. To make the world safe for btree_gist to do this, we'd have
> to add extra lookup activity there as well.
>
> Quite honestly, I think that the bug is that btree_gist took it upon
> itself to invent <> as an indexable operator. The odds that that will
> ever be of practical use seem negligible, and not at all adequate to
> warrant adding extra cycles into mainstream code paths. It's not too
> late to rip that out of 9.1, and that's what I think we should do.
I think that ripping out the change to btree_gist is also insufficient;
we would also have to prevent other extensions from doing the same. That
means documenting an odd special case, and testing for it when defining
an opclass. And then we'd probably have to backpatch this kludge.
Something simpler seems possible here. The root of the problem is that
we're being fooled by GiST opclasses when all we care about are BTree
opclasses anyway. A simple fix would be to introduce a flag
"found_btree_op". If we hit any BTree entries from pg_amop at all, then
we're done after the loop is done. If not, then we negate the op and
loop again.
Would that be acceptable?
Regards,
Jeff Davis
