Re: PATCH: index-only scans with partial indexes

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
> Hello, This is a (maybe) committer-ready patch of a Tomas
> Vondra's project.

I think this needs quite a bit of work yet.  A few comments:

* If we're going to pay the price of identifying implied restriction
conditions in check_partial_indexes(), we should at least recoup some
of that investment by not doing it again in create_indexscan_plan().

* create_indexscan_plan() has this comment about what it's doing:    * We can also discard quals that are implied by a
partialindex's    * predicate, but only in a plain SELECT; when scanning a target relation    * of UPDATE/DELETE/SELECT
FORUPDATE, we must leave such quals in the    * plan so that they'll be properly rechecked by EvalPlanQual testing.
 
I believe that that problem applies for this optimization as well,
and thus that you can only remove implied quals in plain SELECT.
At least, if there's some reason why that problem does *not* apply,
there had darn well better be a comment explaining why it's safe.

* Adding indexrinfos to IndexPath seems unnecessary, since that struct
already has the "index" pointer --- you can just get the field out of the
IndexOptInfo when you need it.  If you insist on having the extra field,
this patch is short of the threshold for correctness on adding fields to
paths.  It missed _outIndexPath for instance.

* The additional #include in costsize.c has no apparent reason.

* The changes in cost_index() really ought to come with some change
in the associated comments.

* Personally I'd not change the signature of
match_restriction_clauses_to_index; that's just code churn that
may have to get undone someday.

* The block comment in check_index_only needs more thought than this,
as the phrase "The same is true" is now invalid; the "same" it refers
to *isn't* the same anymore.

* I'm not too thrilled with injecting the initialization of
index->indrinfos into the initial loop in check_partial_indexes().
If it stays there, I'd certainly expect the comment ahead of the
loop to be changed to have something to do with reality.  But can't
we find some more-appropriate place to initialize it?  Like maybe
where the IndexOptInfo is first created?  I would not really expect
check_partial_indexes() to have side-effects on non-partial indexes.

* I think the double loop in check_partial_indexes() is too cute by half.
I'd be inclined to just build the replacement list unconditionally while
we do the predicate_implied_by() tests.  Those are expensive enough that
saving one lappend per implication-test is a useless optimization,
especially if it requires code as contorted and bug-prone as this.

* The comment added to IndexOptInfo is not very adequate, and not spelled
correctly either.  There's a block comment you should be adding a para to
(probably take the text you added for struct IndexPath).  And again,
there is more work to do to add a field to such a struct, eg outfuncs.c.
Usually a good way to find all the places to touch is to grep for some of
the existing field names in the struct.

* I don't much care for the field name "indrinfos"; it's neither very
readable nor descriptive.  Don't have a better suggestion right now
though.

* Not sure if new regression test cases would be appropriate.  The changes
in the existing cases seem a bit unfortunate actually; I'm afraid that
this may be defeating the original intent of those tests.

I'm setting this back to Waiting on Author.
        regards, tom lane