Re: PATCH: index-only scans with partial indexes

Hi,

On 09/18/2015 03:46 AM, Kyotaro HORIGUCHI wrote:
> Hello,
>
> At Thu, 17 Sep 2015 17:40:27 +0200, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in
<55FADEEB.4000907@2ndquadrant.com>
>> Yes, this seems sane. I've been poking at this a bit too, and I came
>> to the same plan in general, except that I think it's better to build
>> list of clauses that are *not* implied by the index, because that's
>> what we need both in cost_index and check_index_only.
>
> I intended to isolate IndexOptInfo from belonging RelOptInfo but
> the exclusion list also bonds them tightly, and one IndexOptInfo
> belongs to only one RelOptInfo so no need to isolate. So
> not-implied-restrictclauses in IndexOptInfo would be preferable.
>
>> It also seems to me that this change (arguably a bug fix) should
>> pretty much make the original patch irrelevant, because
>> check_index_only can simply walk over the new list.
>
> Yeah. This seems to be a bug irrelevant to your index-only-scan
> ptch.


Attached is a patch that makes this work correctly, I believe. The only
difference compared to the v1 of the patch is this piece of code in
match_clause_to_index (indexpath.c):

if ((index->indpred != NIL) &&
     (predicate_implied_by(lappend(NIL, rinfo->clause), index->indpred)))
     continue;

which effectively says that we should ignore clauses implied by the
index predicate.

This fixes the way we build IndexClauseSet for the two indexes -
originally we'd add the implied clause on the large index but not on the
small one (because it does not have the column referenced in the
condition). And as far as I can say, this also fixes all the costing
issues that I've described because cost_index sees the same thing for
both indexes.

It's also early enough to fix the actual path, so EXPLAIN shows the same
thing for both indexes (so no "Index Cond" present for only one of the
indexes).

I've originally tried to fix this in extract_nonindex_conditions, but
that's way too late - it can only fix the costing, not the path.

Arguably this part of the patch is a bugfix of an issue present on master.

The patch does not change the check_index_only implementation - it still
needs to check the clauses, just like in v1 of the patch. To make this
re-check unnecessary, we'd have to stick the remaining clauses
somewhere, so that check_index_only can use only the filtered list
(instead of walking through the complete list of restrictions).

Or perhaps we can do the check in match_clause_to_index, pretty much for
free? If the clause is not implied by the index predicate (which we know
thanks to the fix), and we don't assign it to any of the index columns,
it means we can'd use IOS, no?

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services