Kyotaro HORIGUCHI <horiguchi.kyot...@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 partial index's * predicate, but only in a plain SELECT; when scanning a target relation * of UPDATE/DELETE/SELECT FOR UPDATE, 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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers