Re: PATCH: index-only scans with partial indexes - Mailing list pgsql-hackers

Hello,

At Tue, 29 Sep 2015 16:57:03 +0200, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote in
<560AA6BF.5030805@2ndquadrant.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
forpartial 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