Re: Use of additional index columns in rows filtering

On 7/18/23 22:21, Jeff Davis wrote:
> Hi,
>
>
> On Sun, 2023-07-16 at 22:36 +0200, Tomas Vondra wrote:
>> This kept bothering me, so I looked at it today, and reworked it to
>> use
>> the IOS approach.
>
> Initial comments on patch 20230716:
>
> * check_index_filter() alredy looks at "canreturn", which should mean
> that you don't need to later check for opcintype<>opckeytype. But
> there's a comment in IndexNext() indicating that's a problem -- under
> what conditions is it a problem?
>

The comment in IndexNext() is a bit obsolete. There was an issue when
using a slot matching the index, because then StoreIndexTuple might fail
because of type mismatch (as explained in [1]). But that's no longer an
issue, thanks to switching to the table slot in the last patch version.

> * (may be a matter of taste) Recomputing the bitmapset from the
> canreturn array in check_index_filter() for each call seems awkward. I
> would just iterate through the bitmapset and check that all are set
> true in the amcanreturn array.
>

check_index_filter() is a simplified version of check_index_only(), and
that calculates the bitmap this way.

> * There are some tiny functions that don't seem to add much value or
> have slightly weird APIs. For instance, match_filter_to_index() could
> probably just return a boolean, and maybe doesn't even need to exist
> because it's such a thin wrapper over check_index_filter(). Similarly
> for fix_indexfilter_clause(). I'm OK with tiny functions even if the
> only value is a comment, but I didn't find these particularly helpful.
>

Yes, I agree some of this could be simplified. I only did the bare
minimum to get this bit working.

> * fix_indexfilter_references() could use a better comment. Perhaps
> refactor so that you can share code with fix_indexqual_references()?
>

I don't think this can share code with fix_indexqual_references(),
because that changes the Var nodes to point to the index (because it
then gets translated to scan keys). The filters don't need that.

> * it looks like index filters are duplicated with ordinary filters, is
> there a reason for that?
>

Good point. That used to be necessary, because the index-only filters
can be evaluated only on all-visible pages, and filters were had Vars
referencing the index tuple. We'd have to maintain another list of
clauses, which didn't seem worth it.

But now that the filters reference the heap tuple, we could not include
them into the second list.

> * I'm confused about the relationship of an IOS to an index filter. It
> seems like the index filter only works for an ordinary index scan? Why
> is that?

What would it do for IOS? IOS evaluates all filters on the index tuple,
and it does not need the heap tuple at all (assuming allvisible=true).

Index-only filters try to do something like that even for regular index
scans, by evaluating as many expression on the index tuple, but may
still require fetching the heap tuple in the end.

regards

[1]
https://www.postgresql.org/message-id/97985ef2-ef9b-e62e-6fd4-e00a573d4ead@enterprisedb.com

--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company