Re: PATCH: index-only scans with partial indexes

Hello,

At Tue, 29 Sep 2015 16:57:03 +0200, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote in <560AA6BF(dot)5030805(at)2ndquadrant(dot)com>
> >>> 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).
> >>
> >> After thinking about this a bit more, I realized we already have a
> >> good place for keeping this list - IndexClauseSet. So I've done that,
> >
> > The place looks fine but it might be better that rclauses have
> > baserestrictinfo itself when indpred == NIL. It would make the
> > semantics of rclauses more consistent.
> >
> >> and removed most of the code from check_index_only - it still needs to
> >> decide whether to use the full list of restrictions (regular indexes)
> >> or the filtered list (for partial indexes).
> >
> > The change above allows to reduce the modification still left in
> > check_index_only.
>
> I'm not sure I understand what change you suggest? Could you explain?
>
> The change in check_index_only is effectively just (a) comment update
> and (b) choice of the right list of clauses. I'd say both changes are
> needed, although (b) could happen outside check_index_only (i.e. we
> could do the check elsewhere). Is that what you mean?

I meant that the (b) could be done earlier in
match_clause_to_index. Currently clauseset->rclauses stores given
(restrict) clauses which are not implied by indpred *only when*
indpred is present. But it's no problem that rclauses *always*
stores such clauses. rclauses is still storing clauses "not
implied by the index" for the case. It is what I meant by "more
consistent".

The following codelet would be more clearer than my crumsy
words:)

in match_clause_to_index()
> if (index->indpred != NIL &&
> predicate_implied_by(list_make1(rinfo->clause), index->indpred))
> return; /* ignore clauses implied by index */
>
> /* track all clauses not implied by indpred */
> clauseset->rclauses = lappend(clauseset->rclauses, rinfo);
>
> for (indexcol = 0; indexcol < index->ncolumns; indexcol++)

in check_index_only()
- * For partial indexes use the filtered clauses, otherwise use the
- * baserestrictinfo directly for non-partial indexes.
- */
- rclauses = (index->indpred != NIL) ? clauses : rel->baserestrictinfo;
-
- /*
* Add all the attributes used by restriction clauses (only those not
* implied by the index predicate for partial indexes).
*/
- foreach(lc, rclauses)
+ foreach(lc, clauses)

> > cost_index() seems to need to be fixed. It would count excluded
> > clauses in estimate.
>
> Hmm, good point. The problem is that extract_nonindex_conditions uses
> baserel->baserestrictinfo again, i.e. it does not skip the implied
> clauses. So we may either stick the filtered clauses somewhere (for
> example in the IndexPath), teach extract_nonindex_conditions to use
> predicate_implied_by. I'd say the first option is better. Agreed?

I'll mention this in a separate reply for the following mail.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center