]> granicus.if.org Git - postgresql/commitdiff
Fix some problems with selectivity estimation for partial indexes.
authorTom Lane <tgl@sss.pgh.pa.us>
Wed, 21 Mar 2007 22:18:12 +0000 (22:18 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Wed, 21 Mar 2007 22:18:12 +0000 (22:18 +0000)
First, genericcostestimate() was being way too liberal about including
partial-index conditions in its selectivity estimate, resulting in
substantial underestimates for situations such as an indexqual "x = 42"
used with an index on x "WHERE x >= 40 AND x < 50".  While the code is
intentionally set up to favor selecting partial indexes when available,
this was too much...

Second, choose_bitmap_and() was likewise easily fooled by cases of this
type, since it would similarly think that the partial index had selectivity
independent of the indexqual.

Fixed by using predicate_implied_by() rather than simple equality checks
to determine redundancy.  This is a good deal more expensive but I don't
see much alternative.  At least the extra cost is only paid when there's
actually a partial index under consideration.

Per report from Jeff Davis.  I'm not going to risk back-patching this,
though.

src/backend/optimizer/path/indxpath.c
src/backend/utils/adt/selfuncs.c
src/test/regress/expected/select.out
src/test/regress/sql/select.sql

index 04e029beb28b7deee2345c27939cd214b48df5ef..7197658ae9bcad72b676d25f540c498a08e69cfc 100644 (file)
@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/optimizer/path/indxpath.c,v 1.217 2007/03/17 00:11:04 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/optimizer/path/indxpath.c,v 1.218 2007/03/21 22:18:12 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -57,8 +57,8 @@ static Path *choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel,
 static int     bitmap_path_comparator(const void *a, const void *b);
 static Cost bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel,
                                        List *paths, RelOptInfo *outer_rel);
-static List *pull_indexpath_quals(Path *bitmapqual);
-static bool lists_intersect_ptr(List *list1, List *list2);
+static void find_indexpath_quals(Path *bitmapqual, List **quals, List **preds);
+static bool lists_intersect(List *list1, List *list2);
 static bool match_clause_to_indexcol(IndexOptInfo *index,
                                                 int indexcol, Oid opfamily,
                                                 RestrictInfo *rinfo,
@@ -562,6 +562,7 @@ choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel,
        Path      **patharray;
        Cost            costsofar;
        List       *qualsofar;
+       List       *firstpred;
        ListCell   *lastcell;
        int                     i;
        ListCell   *l;
@@ -586,8 +587,7 @@ choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel,
         * consider an index redundant if any of its index conditions were already
         * used by earlier indexes.  (We could use predicate_implied_by to have a
         * more intelligent, but much more expensive, check --- but in most cases
-        * simple pointer equality should suffice, since after all the index
-        * conditions are all coming from the same RestrictInfo lists.)
+        * simple equality should suffice.)
         *
         * You might think the condition for redundancy should be "all index
         * conditions already used", not "any", but this turns out to be wrong.
@@ -597,10 +597,27 @@ choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel,
         * non-selective.  In any case, we'd surely be drastically misestimating
         * the selectivity if we count the same condition twice.
         *
-        * We include index predicate conditions in the redundancy test.  Because
-        * the test is just for pointer equality and not equal(), the effect is
-        * that use of the same partial index in two different AND elements is
-        * considered redundant.  (XXX is this too strong?)
+        * We must also consider index predicate conditions in checking for
+        * redundancy, because the estimated selectivity of a partial index
+        * includes its predicate even if the explicit index conditions don't.
+        * Here we have to work harder than just checking expression equality:
+        * we check to see if any of the predicate clauses are implied by
+        * index conditions or predicate clauses of previous paths.  This covers
+        * cases such as a condition "x = 42" used with a plain index, followed
+        * by a clauseless scan of a partial index "WHERE x >= 40 AND x < 50".
+        * Also, we reject indexes that have a qual condition matching any
+        * previously-used index's predicate (by including predicate conditions
+        * into qualsofar).  It should be sufficient to check equality in this
+        * case, not implication, since we've sorted the paths by selectivity
+        * and so tighter conditions are seen first --- only for exactly equal
+        * cases might the partial index come first.
+        *
+        * XXX the reason we need all these redundancy checks is that costsize.c
+        * and clausesel.c aren't very smart about redundant clauses: they will
+        * usually double-count the redundant clauses, producing a too-small
+        * selectivity that makes a redundant AND look like it reduces the total
+        * cost.  Perhaps someday that code will be smarter and we can remove
+        * these heuristics.
         *
         * Note: outputting the selected sub-paths in selectivity order is a good
         * thing even if we weren't using that as part of the selection method,
@@ -619,18 +636,38 @@ choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel,
 
        paths = list_make1(patharray[0]);
        costsofar = bitmap_and_cost_est(root, rel, paths, outer_rel);
-       qualsofar = pull_indexpath_quals(patharray[0]);
+       find_indexpath_quals(patharray[0], &qualsofar, &firstpred);
+       qualsofar = list_concat(qualsofar, firstpred);
        lastcell = list_head(paths);    /* for quick deletions */
 
        for (i = 1; i < npaths; i++)
        {
                Path       *newpath = patharray[i];
                List       *newqual;
+               List       *newpred;
                Cost            newcost;
 
-               newqual = pull_indexpath_quals(newpath);
-               if (lists_intersect_ptr(newqual, qualsofar))
+               find_indexpath_quals(newpath, &newqual, &newpred);
+               if (lists_intersect(newqual, qualsofar))
                        continue;                       /* consider it redundant */
+               if (newpred)
+               {
+                       bool    redundant = false;
+
+                       /* we check each predicate clause separately */
+                       foreach(l, newpred)
+                       {
+                               Node       *np = (Node *) lfirst(l);
+
+                               if (predicate_implied_by(list_make1(np), qualsofar))
+                               {
+                                       redundant = true;
+                                       break;          /* out of inner loop */
+                               }
+                       }
+                       if (redundant)
+                               continue;
+               }
                /* tentatively add newpath to paths, so we can estimate cost */
                paths = lappend(paths, newpath);
                newcost = bitmap_and_cost_est(root, rel, paths, outer_rel);
@@ -638,7 +675,7 @@ choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel,
                {
                        /* keep newpath in paths, update subsidiary variables */
                        costsofar = newcost;
-                       qualsofar = list_concat(qualsofar, newqual);
+                       qualsofar = list_concat(list_concat(qualsofar, newqual), newpred);
                        lastcell = lnext(lastcell);
                }
                else
@@ -710,35 +747,39 @@ bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel,
 }
 
 /*
- * pull_indexpath_quals
+ * find_indexpath_quals
  *
- * Given the Path structure for a plain or bitmap indexscan, extract a list
+ * Given the Path structure for a plain or bitmap indexscan, extract lists
  * of all the indexquals and index predicate conditions used in the Path.
  *
  * This is sort of a simplified version of make_restrictinfo_from_bitmapqual;
  * here, we are not trying to produce an accurate representation of the AND/OR
  * semantics of the Path, but just find out all the base conditions used.
  *
- * The result list contains pointers to the expressions used in the Path,
+ * The result lists contain pointers to the expressions used in the Path,
  * but all the list cells are freshly built, so it's safe to destructively
- * modify the list (eg, by concat'ing it with other lists).
+ * modify the lists (eg, by concat'ing with other lists).
  */
-static List *
-pull_indexpath_quals(Path *bitmapqual)
+static void
+find_indexpath_quals(Path *bitmapqual, List **quals, List **preds)
 {
-       List       *result = NIL;
        ListCell   *l;
 
+       *quals = NIL;
+       *preds = NIL;
+
        if (IsA(bitmapqual, BitmapAndPath))
        {
                BitmapAndPath *apath = (BitmapAndPath *) bitmapqual;
 
                foreach(l, apath->bitmapquals)
                {
-                       List       *sublist;
+                       List       *subquals;
+                       List       *subpreds;
 
-                       sublist = pull_indexpath_quals((Path *) lfirst(l));
-                       result = list_concat(result, sublist);
+                       find_indexpath_quals((Path *) lfirst(l), &subquals, &subpreds);
+                       *quals = list_concat(*quals, subquals);
+                       *preds = list_concat(*preds, subpreds);
                }
        }
        else if (IsA(bitmapqual, BitmapOrPath))
@@ -747,36 +788,36 @@ pull_indexpath_quals(Path *bitmapqual)
 
                foreach(l, opath->bitmapquals)
                {
-                       List       *sublist;
+                       List       *subquals;
+                       List       *subpreds;
 
-                       sublist = pull_indexpath_quals((Path *) lfirst(l));
-                       result = list_concat(result, sublist);
+                       find_indexpath_quals((Path *) lfirst(l), &subquals, &subpreds);
+                       *quals = list_concat(*quals, subquals);
+                       *preds = list_concat(*preds, subpreds);
                }
        }
        else if (IsA(bitmapqual, IndexPath))
        {
                IndexPath  *ipath = (IndexPath *) bitmapqual;
 
-               result = get_actual_clauses(ipath->indexclauses);
-               result = list_concat(result, list_copy(ipath->indexinfo->indpred));
+               *quals = get_actual_clauses(ipath->indexclauses);
+               *preds = list_copy(ipath->indexinfo->indpred);
        }
        else
                elog(ERROR, "unrecognized node type: %d", nodeTag(bitmapqual));
-
-       return result;
 }
 
 
 /*
- * lists_intersect_ptr
+ * lists_intersect
  *             Detect whether two lists have a nonempty intersection,
- *             using pointer equality to compare members.
+ *             using equal() to compare members.
  *
  * This possibly should go into list.c, but it doesn't yet have any use
  * except in choose_bitmap_and.
  */
 static bool
-lists_intersect_ptr(List *list1, List *list2)
+lists_intersect(List *list1, List *list2)
 {
        ListCell   *cell1;
 
@@ -787,7 +828,7 @@ lists_intersect_ptr(List *list1, List *list2)
 
                foreach(cell2, list2)
                {
-                       if (lfirst(cell2) == datum1)
+                       if (equal(lfirst(cell2), datum1))
                                return true;
                }
        }
index df61fea567b7e76db22cca5caa5e75dec74140ad..a92317aeac1a96bbc3617ae64c567878a91486f3 100644 (file)
@@ -15,7 +15,7 @@
  *
  *
  * IDENTIFICATION
- *       $PostgreSQL: pgsql/src/backend/utils/adt/selfuncs.c,v 1.229 2007/03/17 00:11:05 tgl Exp $
+ *       $PostgreSQL: pgsql/src/backend/utils/adt/selfuncs.c,v 1.230 2007/03/21 22:18:12 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -86,6 +86,7 @@
 #include "optimizer/pathnode.h"
 #include "optimizer/paths.h"
 #include "optimizer/plancat.h"
+#include "optimizer/predtest.h"
 #include "optimizer/restrictinfo.h"
 #include "optimizer/var.h"
 #include "parser/parse_coerce.h"
@@ -4775,36 +4776,38 @@ genericcostestimate(PlannerInfo *root,
        List       *selectivityQuals;
        ListCell   *l;
 
-       /*
+       /*----------
         * If the index is partial, AND the index predicate with the explicitly
         * given indexquals to produce a more accurate idea of the index
-        * selectivity.  This may produce redundant clauses.  We get rid of exact
-        * duplicates in the code below.  We expect that most cases of partial
-        * redundancy (such as "x < 4" from the qual and "x < 5" from the
-        * predicate) will be recognized and handled correctly by
-        * clauselist_selectivity().  This assumption is somewhat fragile, since
-        * it depends on predicate_implied_by() and clauselist_selectivity()
-        * having similar capabilities, and there are certainly many cases where
-        * we will end up with a too-low selectivity estimate.  This will bias the
-        * system in favor of using partial indexes where possible, which is not
-        * necessarily a bad thing. But it'd be nice to do better someday.
+        * selectivity.  However, we need to be careful not to insert redundant
+        * clauses, because clauselist_selectivity() is easily fooled into
+        * computing a too-low selectivity estimate.  Our approach is to add
+        * only the index predicate clause(s) that cannot be proven to be implied
+        * by the given indexquals.  This successfully handles cases such as a
+        * qual "x = 42" used with a partial index "WHERE x >= 40 AND x < 50".
+        * There are many other cases where we won't detect redundancy, leading
+        * to a too-low selectivity estimate, which will bias the system in favor
+        * of using partial indexes where possible.  That is not necessarily bad
+        * though.
         *
-        * Note that index->indpred and indexQuals are both in implicit-AND form,
-        * so ANDing them together just takes merging the lists.  However,
-        * eliminating duplicates is a bit trickier because indexQuals contains
-        * RestrictInfo nodes and the indpred does not.  It is okay to pass a
-        * mixed list to clauselist_selectivity, but we have to work a bit to
-        * generate a list without logical duplicates.  (We could just list_union
-        * indpred and strippedQuals, but then we'd not get caching of per-qual
-        * selectivity estimates.)
+        * Note that indexQuals contains RestrictInfo nodes while the indpred
+        * does not.  This is OK for both predicate_implied_by() and
+        * clauselist_selectivity().
+        *----------
         */
        if (index->indpred != NIL)
        {
-               List       *strippedQuals;
-               List       *predExtraQuals;
+               List       *predExtraQuals = NIL;
+
+               foreach(l, index->indpred)
+               {
+                       Node   *predQual = (Node *) lfirst(l);
+                       List   *oneQual = list_make1(predQual);
 
-               strippedQuals = get_actual_clauses(indexQuals);
-               predExtraQuals = list_difference(index->indpred, strippedQuals);
+                       if (!predicate_implied_by(oneQual, indexQuals))
+                               predExtraQuals = list_concat(predExtraQuals, oneQual);
+               }
+               /* list_concat avoids modifying the passed-in indexQuals list */
                selectivityQuals = list_concat(predExtraQuals, indexQuals);
        }
        else
index 0b3f546bdfbcf2d30cb5704f42c6e79fb0633fb5..d58c8d2811be2f598d9ae25819f5cb4f63b6476a 100644 (file)
@@ -209,9 +209,13 @@ SELECT onek.unique1, onek.string4 FROM onek
 -- test partial btree indexes
 --
 -- As of 7.2, planner probably won't pick an indexscan without stats,
--- so ANALYZE first.
+-- so ANALYZE first.  Also, we want to prevent it from picking a bitmapscan
+-- followed by sort, because that could hide index ordering problems.
 --
 ANALYZE onek2;
+SET enable_seqscan TO off;
+SET enable_bitmapscan TO off;
+SET enable_sort TO off;
 --
 -- awk '{if($1<10){print $0;}else{next;}}' onek.data | sort +0n -1
 --
@@ -288,6 +292,9 @@ SELECT onek2.unique1, onek2.stringu1 FROM onek2
      999 | LMAAAA
 (19 rows)
 
+RESET enable_seqscan;
+RESET enable_bitmapscan;
+RESET enable_sort;
 SELECT two, stringu1, ten, string4
    INTO TABLE tmp
    FROM onek;
index f23cccd24f9a6d2502fea369d9536bd870cf9fe7..8c92168c9b88f08276874514617f114d02b4e5ea 100644 (file)
@@ -59,10 +59,15 @@ SELECT onek.unique1, onek.string4 FROM onek
 -- test partial btree indexes
 --
 -- As of 7.2, planner probably won't pick an indexscan without stats,
--- so ANALYZE first.
+-- so ANALYZE first.  Also, we want to prevent it from picking a bitmapscan
+-- followed by sort, because that could hide index ordering problems.
 --
 ANALYZE onek2;
 
+SET enable_seqscan TO off;
+SET enable_bitmapscan TO off;
+SET enable_sort TO off;
+
 --
 -- awk '{if($1<10){print $0;}else{next;}}' onek.data | sort +0n -1
 --
@@ -81,6 +86,10 @@ SELECT onek2.unique1, onek2.stringu1 FROM onek2
 SELECT onek2.unique1, onek2.stringu1 FROM onek2
    WHERE onek2.unique1 > 980;
 
+RESET enable_seqscan;
+RESET enable_bitmapscan;
+RESET enable_sort;
+
 
 SELECT two, stringu1, ten, string4
    INTO TABLE tmp