Bug: When user-defined AM is used, the index path cannot be selected correctly

Hi,

1. When using extended PGroonga

CREATE EXTENSION pgroonga;

CREATE TABLE memos (
id boolean,
content varchar
);

CREATE INDEX idxA ON memos USING pgroonga (id);

2. Disable bitmapscan and seqscan:

SET enable_seqscan=off;
SET enable_indexscan=on;
SET enable_bitmapscan=off;

3. Neither ID = 'f' nor id= 't' can use the index correctly.

postgres=# explain select * from memos where id='f';
QUERY PLAN
--------------------------------------------------------------------------
Seq Scan on memos (cost=10000000000.00..10000000001.06 rows=3 width=33)
Filter: (NOT id)
(2 rows)

postgres=# explain select * from memos where id='t';
QUERY PLAN
--------------------------------------------------------------------------
Seq Scan on memos (cost=10000000000.00..10000000001.06 rows=3 width=33)
Filter: id
(2 rows)

postgres=# explain select * from memos where id>='t';
QUERY PLAN
-------------------------------------------------------------------
Index Scan using idxa on memos (cost=0.00..4.01 rows=2 width=33)
Index Cond: (id >= true)
(2 rows)

The reason is that these expressions are converted to BoolExpr and Var.
match_clause_to_indexcol does not use them to check boolean-index.

patch attached.

--
Quan Zongliang
Beijing Vastdata

Quan Zongliang <quanzongliang(at)yeah(dot)net> writes:
> 1. When using extended PGroonga
> ...
> 3. Neither ID = 'f' nor id= 't' can use the index correctly.

This works fine for btree indexes. I think the actual problem
is that IsBooleanOpfamily only accepts the btree and hash
opclasses, and that's what needs to be improved. Your proposed
patch fails to do that, which makes it just a crude hack that
solves some aspects of the issue (and probably breaks other
things).

It might work to change IsBooleanOpfamily so that it checks to
see whether BooleanEqualOperator is a member of the opclass.
That's basically what we need to know before we dare generate
substitute index clauses. It's kind of an expensive test
though, and the existing coding assumes that IsBooleanOpfamily
is cheap ...

regards, tom lane

On 2022/8/17 10:03, Tom Lane wrote:
> Quan Zongliang <quanzongliang(at)yeah(dot)net> writes:
>> 1. When using extended PGroonga
>> ...
>> 3. Neither ID = 'f' nor id= 't' can use the index correctly.
>
> This works fine for btree indexes. I think the actual problem
> is that IsBooleanOpfamily only accepts the btree and hash
> opclasses, and that's what needs to be improved. Your proposed
> patch fails to do that, which makes it just a crude hack that
> solves some aspects of the issue (and probably breaks other
> things).
>
> It might work to change IsBooleanOpfamily so that it checks to
> see whether BooleanEqualOperator is a member of the opclass.
> That's basically what we need to know before we dare generate
> substitute index clauses. It's kind of an expensive test
> though, and the existing coding assumes that IsBooleanOpfamily
> is cheap ...
>
> regards, tom lane

New patch attached.

It seems that partitions do not use AM other than btree and hash.
Rewrite only indxpath.c and check if it is a custom AM.

Quan Zongliang <quanzongliang(at)yeah(dot)net> writes:
> New patch attached.
> It seems that partitions do not use AM other than btree and hash.
> Rewrite only indxpath.c and check if it is a custom AM.

This seems drastically underdocumented, and the test you're using
for extension opclasses is wrong. What we need to know before
applying match_boolean_index_clause is that a clause using
BooleanEqualOperator will be valid for the index. That's got next
door to nothing to do with whether the opclass is default for
the index AM. A non-default opclass might support that operator,
and conversely a default one might not (although I concede it's
not that easy to imagine what other set of operators-on-boolean
an extension opclass might be interested in).

I think we need something more like the attached.

regards, tom lane

I wrote:
> I think we need something more like the attached.

Meh -- serves me right for not doing check-world before sending.
The patch causes some plans to change in the btree_gin and btree_gist
modules; which is good, because that shows that the patch is actually
doing what it's supposed to. The fact that your patch didn't make
the cfbot unhappy implies that it wasn't triggering for those modules.
I think the reason is that you did

+ ((amid) >= FirstNormalObjectId && \
+ OidIsValid(GetDefaultOpClass(BOOLOID, (amid)))) \

so that the FirstNormalObjectId cutoff was applied to the AM's OID,
not to the opfamily OID, causing it to do the wrong thing for
extension opclasses over built-in AMs.

The good news is this means we don't need to worry about making
a test case ...

regards, tom lane