Hi, At Sat, 26 Sep 2015 18:00:33 +0200, Tomas Vondra <tomas.von...@2ndquadrant.com> wrote in <5606c121.10...@2ndquadrant.com> > Hi, > > On 09/26/2015 01:28 PM, Tomas Vondra wrote: > > > 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. > Calling predicate_implied_by in match_clause_to_index makes the check > a bit earlier, compared to the v1 of the patch. So theoretically there > might be cases where we'd interrupt the execution between those > places, and the v1 would not pay the price for the call. But those > places are fairly close together, so I find that unlikely and I've > been unable to reproduce a plausible example of such regression > despite trying. > > The only regression test this breaks is "aggregates", and I believe > that's actually correct because it changes the plans like this: > > -> Index Only Scan Backward using minmaxtest2i on minmaxtest2 > Index Cond: (f1 IS NOT NULL) > -> Index Only Scan using minmaxtest3i on minmaxtest3 > - Index Cond: (f1 IS NOT NULL) > > i.e. it removes the Index Condition from scans of minmaxtest3i, which > is perfectly sensible because the index is defined like this: > > create index minmaxtest3i on minmaxtest3(f1) where f1 is not null; > > So it's partial index and that condition is implied by the predicate > (it's actually exactly the same). Agreed. It looks an unexpected-but-positive product. > I haven't fixed those regression tests for now. > > > > > Or perhaps we can do the check in match_clause_to_index, pretty much > > for free? If the clause is not implied by the index predicate (which > > we know thanks to the fix), and we don't assign it to any of the > > index columns, it means we can'd use IOS, no? > > After thinking about this a bit more, this is clearly nonsense. It > fails on conditions referencing multiple columns (WHERE a=b) which > can't be assigned to a single index column. So the logic would have to > be much more complicated, effectively doing what check_index_only is > doing just a tiny bit later. cost_index() seems to need to be fixed. It would count excluded clauses in estimate. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
