Hello, it looks fine.
> >> 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?
>
> And the attached patch v4 should do the trick - it adds 'indexrinfos'
> to IndexPath and uses it in cost_index().
It seems to work as expected. It'd also be reasonable to have
"indexrinfos" in IndexPath since cost_index needs it. Loose join
clauses and recheck conditions are not broken.
By the way your comment for indexrinfos is as following,
> * 'indexrinfos' is a list of RestrictInfo nodes from the query's WHERE
> * or JOIN conditions, excluding those implied by the index predicate
> * (if the index is not partial, the list includes all restriction clauses).
But the v4 patch instead leaves it empty for non-partial
indexes:) I prefer to follow this comment because looking the
condition (index->indpred != NIL) for such purpose in
build_index_paths is somewhat uneasy for me. But I don't insist
on that if you choose to avoid useless memory and clock
consumption to construct a list which is not so meaningful for
non-partial indexes (it is almost all cases).
The following comment in match_clause_to_index does not need to
be a 'XXX' comment. What made you to feel to do so? (I rather
feel that it is not necessary at all.)
> * XXX We must do this before trying to match the index to index
> * columns, because the index predicates may use columns not
> * used in the index itself.
Anyway some description on rclauses should be added in the
comment for match_clause_to_index, instead of the comments
currently added *within* the function.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
