Fix long-obsolete code for separating filter conditions in cost_index().

This code relied on pointer equality to identify which restriction clauses
also appear in the indexquals (and, therefore, don't need to be applied as
simple filter conditions).  That was okay once upon a time, years ago,
before we introduced the equivalence-class machinery.  Now there's about a
50-50 chance that an equality clause appearing in the indexquals will be
the mirror image (commutator) of its mate in the restriction list.  When
that happens, we'd erroneously think that the clause would be re-evaluated
at each visited row, and therefore inflate the cost estimate for the
indexscan by the clause's cost.

Add some logic to catch this case.  It seems to me that it continues not to
be worthwhile to expend the extra predicate-proof work that createplan.c
will do on the finally-selected plan, but this case is common enough and
cheap enough to handle that we should do so.

This will make a small difference (about one cpu_operator_cost per row)
in simple cases; but in situations where there's an expensive function in
the indexquals, it can make a very large difference, as seen in recent
example from Jeff Janes.

This is a long-standing bug, but I'm hesitant to back-patch because of the
possibility of destabilizing plan choices that people may be happy with.

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 78ef22949a6bf3c958cee53f0ab9bdcb6a93561f..5a9daf0f207225fafd2fd05cfd1fb5270e6116db 100644 (file)
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -124,6 +124,7 @@ typedef struct
    QualCost    total;
 } cost_qual_eval_context;
 
+static List *extract_nonindex_conditions(List *qual_clauses, List *indexquals);
 static MergeScanSelCache *cached_scansel(PlannerInfo *root,
               RestrictInfo *rinfo,
               PathKey *pathkey);
@@ -242,7 +243,7 @@ cost_index(IndexPath *path, PlannerInfo *root, double loop_count)
    IndexOptInfo *index = path->indexinfo;
    RelOptInfo *baserel = index->rel;
    bool        indexonly = (path->path.pathtype == T_IndexOnlyScan);
-   List       *allclauses;
+   List       *qpquals;
    Cost        startup_cost = 0;
    Cost        run_cost = 0;
    Cost        indexStartupCost;
@@ -265,19 +266,26 @@ cost_index(IndexPath *path, PlannerInfo *root, double loop_count)
    Assert(baserel->relid > 0);
    Assert(baserel->rtekind == RTE_RELATION);
 
-   /* Mark the path with the correct row estimate */
+   /*
+    * Mark the path with the correct row estimate, and identify which quals
+    * will need to be enforced as qpquals.
+    */
    if (path->path.param_info)
    {
        path->path.rows = path->path.param_info->ppi_rows;
-       /* also get the set of clauses that should be enforced by the scan */
-       allclauses = list_concat(list_copy(path->path.param_info->ppi_clauses),
-                                baserel->baserestrictinfo);
+       /* qpquals come from the rel's restriction clauses and ppi_clauses */
+       qpquals = list_concat(
+                      extract_nonindex_conditions(baserel->baserestrictinfo,
+                                                  path->indexquals),
+             extract_nonindex_conditions(path->path.param_info->ppi_clauses,
+                                         path->indexquals));
    }
    else
    {
        path->path.rows = baserel->rows;
-       /* allclauses should just be the rel's restriction clauses */
-       allclauses = baserel->baserestrictinfo;
+       /* qpquals come from just the rel's restriction clauses */
+       qpquals = extract_nonindex_conditions(baserel->baserestrictinfo,
+                                             path->indexquals);
    }
 
    if (!enable_indexscan)
@@ -433,19 +441,9 @@ cost_index(IndexPath *path, PlannerInfo *root, double loop_count)
     * Estimate CPU costs per tuple.
     *
     * What we want here is cpu_tuple_cost plus the evaluation costs of any
-    * qual clauses that we have to evaluate as qpquals.  We approximate that
-    * list as allclauses minus any clauses appearing in indexquals.  (We
-    * assume that pointer equality is enough to recognize duplicate
-    * RestrictInfos.)  This method neglects some considerations such as
-    * clauses that needn't be checked because they are implied by a partial
-    * index's predicate.  It does not seem worth the cycles to try to factor
-    * those things in at this stage, even though createplan.c will take pains
-    * to remove such unnecessary clauses from the qpquals list if this path
-    * is selected for use.
-    */
-   cost_qual_eval(&qpqual_cost,
-                  list_difference_ptr(allclauses, path->indexquals),
-                  root);
+    * qual clauses that we have to evaluate as qpquals.
+    */
+   cost_qual_eval(&qpqual_cost, qpquals, root);
 
    startup_cost += qpqual_cost.startup;
    cpu_per_tuple = cpu_tuple_cost + qpqual_cost.per_tuple;
@@ -456,6 +454,46 @@ cost_index(IndexPath *path, PlannerInfo *root, double loop_count)
    path->path.total_cost = startup_cost + run_cost;
 }
 
+/*
+ * extract_nonindex_conditions
+ *
+ * Given a list of quals to be enforced in an indexscan, extract the ones that
+ * will have to be applied as qpquals (ie, the index machinery won't handle
+ * them).  The actual rules for this appear in create_indexscan_plan() in
+ * createplan.c, but the full rules are fairly expensive and we don't want to
+ * go to that much effort for index paths that don't get selected for the
+ * final plan.  So we approximate it as quals that don't appear directly in
+ * indexquals and also are not redundant children of the same EquivalenceClass
+ * as some indexqual.  This method neglects some infrequently-relevant
+ * considerations such as clauses that needn't be checked because they are
+ * implied by a partial index's predicate.  It does not seem worth the cycles
+ * to try to factor those things in at this stage, even though createplan.c
+ * will take pains to remove such unnecessary clauses from the qpquals list if
+ * this path is selected for use.
+ */
+static List *
+extract_nonindex_conditions(List *qual_clauses, List *indexquals)
+{
+   List       *result = NIL;
+   ListCell   *lc;
+
+   foreach(lc, qual_clauses)
+   {
+       RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
+
+       Assert(IsA(rinfo, RestrictInfo));
+       if (rinfo->pseudoconstant)
+           continue;           /* we may drop pseudoconstants here */
+       if (list_member_ptr(indexquals, rinfo))
+           continue;           /* simple duplicate */
+       if (is_redundant_derived_clause(rinfo, indexquals))
+           continue;           /* derived from same EquivalenceClass */
+       /* ... skip the predicate proof attempts createplan.c will try ... */
+       result = lappend(result, rinfo);
+   }
+   return result;
+}
+
 /*
  * index_pages_fetched
  *   Estimate the number of pages actually fetched after accounting for

diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 76ba1bfa8d28fcbea8421c44b32d9daa624fdcc4..cb69c03df000887276d52c37e64fa15e2bad9bf7 100644 (file)
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -1210,6 +1210,9 @@ create_indexscan_plan(PlannerInfo *root,
     * 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.
+    *
+    * Note: if you change this bit of code you should also look at
+    * extract_nonindex_conditions() in costsize.c.
     */
    qpqual = NIL;
    foreach(l, scan_clauses)