Re: PG regression with row comparison when btree_gist is enabled (BUG)

[...]
> test=# select row(1,2) <> row(1,2);
>
> ERROR: could not determine interpretation of row comparison operator <>
> LINE 1: select row(1,2) <> row(1,2);
> ^
> HINT: Row comparison operators must be associated with btree
operator > families.

Hi Denis,

I don't get this error with 9.1beta2,

$ uname -a
Linux saturn 2.6.35.13-92.fc14.x86_64 #1 SMP Sat May 21 17:26:25 UTC
2011 x86_64 x86_64 x86_64 GNU/Linux
$ psql
psql (9.1beta2)
Type "help" for help.

gavin=> select row(1,2) <> row(1,2);
?column?
----------
f
(1 row)

Cheers,
Gavin

I've actually traced it down to btree_gist. Not sure if this is the expected behavior:

test =# select row(1,2) <> row(1,2);
ERROR:  could not determine interpretation of row comparison operator <>
LINE 1: select row(1,2) <> row(1,2);
                        ^
HINT:  Row comparison operators must be associated with btree operator families.

test=# drop extension btree_gist cascade;
NOTICE:  drop cascades to constraint test_period_excl on table test.test_revs
DROP EXTENSION
test =# select row(1,2) <> row(1,2);
 ?column? 
----------
 f
(1 row)

>________________________________
>From: Gavin Flower <GavinFlower(at)archidevsys(dot)co(dot)nz>
>To: ddebernardy(at)yahoo(dot)com
>Cc: pgsql-testers(at)postgresql(dot)org
>Sent: Wednesday, June 15, 2011 4:40 AM
>Subject: Re: [TESTERS] PG 9.1 regression / row comparison?
>
>[...]
>> test=# select row(1,2) <> row(1,2);
>>
>> ERROR:  could not determine interpretation of row comparison operator <>
>> LINE 1: select row(1,2) <> row(1,2);
>>                        ^
>> HINT:  Row comparison operators must be associated with btree operator > families.
>
>Hi Denis,
>
>I don't get this error with 9.1beta2,
>
>$ uname -a
>Linux saturn 2.6.35.13-92.fc14.x86_64 #1 SMP Sat May 21 17:26:25 UTC 2011 x86_64 x86_64 x86_64 GNU/Linux
>$ psql
>psql (9.1beta2)
>Type "help" for help.
>
>gavin=> select row(1,2) <> row(1,2);
>?column?
>----------
>f
>(1 row)
>
>
>Cheers,
>Gavin
>-
>HOWTO Alpha/Beta Test:
>http://wiki.postgresql.org/wiki/HowToBetaTest
>To make changes to your subscription:
>http://www.postgresql.org/mailpref/pgsql-testers
>
>
>

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:
>
> /*
> * If we can't find any opfamily containing the op, perhaps it is a
> <>
> * operator. See if it has a negator that is in an
> opfamily.
> */
> op_negated = false;
> if (catlist->n_members == 0)
>
>
> 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.

Regards,
Jeff Davis

I only did some cursory tests, but the patch (applied to Macport's beta2 distribution) seems to be working on my dev box (OSX / Snow Leopard).

I'll report back if I run into oddities further down the road.

Thanks a lot!
Denis

>________________________________
>From: Jeff Davis <pgsql(at)j-davis(dot)com>
>To: oleg(at)sai(dot)msu(dot)su
>Cc: Denis de Bernardy <ddebernardy(at)yahoo(dot)com>; Teodor Sigaev <teodor(at)sigaev(dot)ru>; pgsql-bugs(at)postgresql(dot)org
>Sent: Sunday, June 19, 2011 7:23 PM
>Subject: Re: PG regression with row comparison when btree_gist is enabled (BUG)
>
>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:
>>
>>  /*                                                                                               
>>    * If we can't find any opfamily containing the op, perhaps it is a
>> <>                           
>>    * operator.  See if it has a negator that is in an
>> opfamily.                                     
>>    */
>>  op_negated = false;
>>  if (catlist->n_members == 0)
>>
>>
>> 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.
>
>Regards,
>    Jeff Davis
>
>
>

Jeff Davis <pgsql(at)j-davis(dot)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.

regards, tom lane

On Sat, 2011-07-02 at 18:38 -0400, Tom Lane wrote:
> Quite honestly, I think that the bug is that btree_gist took it upon
> itself to invent <> as an indexable operator.

Well, it was following documentation that any extension could
potentially use. I think it's a stretch to call it a bug in anything
other than postgres.

So I think we'd need to introduce extra documentation to say that at
most one of an operator and its negator can be indexable; and we should
add a check to prevent that as well.

> 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.

Fair enough. I think it was kind of an interesting use case, and there
might be others like it; but I agree that it wasn't particularly
compelling.

The alternatives don't seem very attractive. To get it to work with one
lookup we'd have to either clutter the btree opclasses with "<>", or
invent a new syscache that has the AM as a key so we can filter out
non-btree opclasses.

I suppose this is another argument for type interfaces.

Regards,
Jeff Davis

On Sat, 2011-07-02 at 18:38 -0400, Tom Lane wrote:
> Jeff Davis <pgsql(at)j-davis(dot)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

Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> 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.

There is that. I doubt it's worth back-patching, though.

> 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.

Yeah, I had been thinking along the same lines. It will require
duplicating the search loop, which is a bit annoying, but perhaps that
could be factored out as a subroutine.

regards, tom lane

On Tue, 2011-07-05 at 13:56 -0400, Tom Lane wrote:
> Yeah, I had been thinking along the same lines. It will require
> duplicating the search loop, which is a bit annoying, but perhaps that
> could be factored out as a subroutine.

Patch attached. The logic in predtest.c was a little more complex, and I
don't think we have a lot of test coverage in this area (and I didn't
see an easy way to add many tests), so this will need some review.

Regards,
Jeff Davis

Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> On Tue, 2011-07-05 at 13:56 -0400, Tom Lane wrote:
>> Yeah, I had been thinking along the same lines. It will require
>> duplicating the search loop, which is a bit annoying, but perhaps that
>> could be factored out as a subroutine.

> Patch attached. The logic in predtest.c was a little more complex, and I
> don't think we have a lot of test coverage in this area (and I didn't
> see an easy way to add many tests), so this will need some review.

Actually, I'd just been working on this myself. I think the cleanest
solution will be to get rid of the duplicative logic by making
predtest.c use get_op_btree_interpretation(). That will require
changing get_op_btree_interpretation() so it can return amoplefttype
and amoprighttype too, but given the small number of callers, an API
change for it doesn't seem like a problem.

regards, tom lane

On Wed, 2011-07-06 at 13:25 -0400, Tom Lane wrote:
> Actually, I'd just been working on this myself. I think the cleanest
> solution will be to get rid of the duplicative logic by making
> predtest.c use get_op_btree_interpretation(). That will require
> changing get_op_btree_interpretation() so it can return amoplefttype
> and amoprighttype too, but given the small number of callers, an API
> change for it doesn't seem like a problem.

Sounds good to me.

Regards,
Jeff Davis

Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> On Wed, 2011-07-06 at 13:25 -0400, Tom Lane wrote:
>> Actually, I'd just been working on this myself. I think the cleanest
>> solution will be to get rid of the duplicative logic by making
>> predtest.c use get_op_btree_interpretation(). That will require
>> changing get_op_btree_interpretation() so it can return amoplefttype
>> and amoprighttype too, but given the small number of callers, an API
>> change for it doesn't seem like a problem.

> Sounds good to me.

Committed that way. Just for the archives' sake, the test case I was
using for the planner side of things was

create index tenk1p on tenk1 (twothousand) where twothousand <> 42;
explain select * from tenk1 where twothousand < 40;

which should use the partial index, but it was failing to prove the
implication when btree_gist was installed.

regards, tom lane