Fix thinko in recent changes to handle ScalarArrayOpExpr as an indexable
authorTom Lane <tgl@sss.pgh.pa.us>
Thu, 18 May 2006 17:12:10 +0000 (17:12 +0000)
committerTom Lane <tgl@sss.pgh.pa.us>
Thu, 18 May 2006 17:12:10 +0000 (17:12 +0000)
condition: when there are multiple possible index paths involving
ScalarArrayOpExprs, they are logically to be ANDed together not ORed.
This thinko was a direct consequence of trying to put the processing
inside generate_bitmap_or_paths(), which I now see was a bit too cute.
So pull it out and make the callers do it separately (there are only two
that need it anyway).  Partially responds to bug #2441 from Arjen van der Meijden.
There are some additional infelicities exposed by his example, but they
are also in 8.1.x, while this mistake is not.

src/backend/optimizer/path/indxpath.c

index f5c4dcf18aabde41a0564f09f24bc0a9d7524492..935494e077854e979ef8d4fdd9f6a1233a5f4d94 100644 (file)
@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *   $PostgreSQL: pgsql/src/backend/optimizer/path/indxpath.c,v 1.204 2006/04/09 18:18:41 tgl Exp $
+ *   $PostgreSQL: pgsql/src/backend/optimizer/path/indxpath.c,v 1.205 2006/05/18 17:12:10 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -50,6 +50,10 @@ static List *find_usable_indexes(PlannerInfo *root, RelOptInfo *rel,
                    bool istoplevel, bool isjoininner,
                    Relids outer_relids,
                    SaOpControl saop_control);
+static List *find_saop_paths(PlannerInfo *root, RelOptInfo *rel,
+               List *clauses, List *outer_clauses,
+               bool istoplevel, bool isjoininner,
+               Relids outer_relids);
 static Path *choose_bitmap_and(PlannerInfo *root, RelOptInfo *rel, List *paths);
 static int bitmap_path_comparator(const void *a, const void *b);
 static Cost bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel, List *paths);
@@ -189,6 +193,15 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
                                          false, NULL);
    bitindexpaths = list_concat(bitindexpaths, indexpaths);
 
+   /*
+    * Likewise, generate paths using ScalarArrayOpExpr clauses; these can't
+    * be simple indexscans but they can be used in bitmap scans.
+    */
+   indexpaths = find_saop_paths(root, rel,
+                                rel->baserestrictinfo, NIL,
+                                true, false, NULL);
+   bitindexpaths = list_concat(bitindexpaths, indexpaths);
+
    /*
     * If we found anything usable, generate a BitmapHeapPath for the most
     * promising combination of bitmap index paths.
@@ -279,10 +292,6 @@ find_usable_indexes(PlannerInfo *root, RelOptInfo *rel,
         * predOK index to an arm of an OR, which would be a legal but
         * pointlessly inefficient plan.  (A better plan will be generated by
         * just scanning the predOK index alone, no OR.)
-        *
-        * If saop_control is SAOP_REQUIRE and istoplevel is false, the caller
-        * is only interested in indexquals involving ScalarArrayOps, so don't
-        * set useful_predicate to true.
         */
        useful_predicate = false;
        if (index->indpred != NIL)
@@ -308,8 +317,7 @@ find_usable_indexes(PlannerInfo *root, RelOptInfo *rel,
                if (!predicate_implied_by(index->indpred, all_clauses))
                    continue;   /* can't use it at all */
 
-               if (saop_control != SAOP_REQUIRE &&
-                   !predicate_implied_by(index->indpred, outer_clauses))
+               if (!predicate_implied_by(index->indpred, outer_clauses))
                    useful_predicate = true;
            }
        }
@@ -398,11 +406,54 @@ find_usable_indexes(PlannerInfo *root, RelOptInfo *rel,
 }
 
 
+/*
+ * find_saop_paths
+ *     Find all the potential indexpaths that make use of ScalarArrayOpExpr
+ *     clauses.  The executor only supports these in bitmap scans, not
+ *     plain indexscans, so we need to segregate them from the normal case.
+ *     Otherwise, same API as find_usable_indexes().
+ *     Returns a list of IndexPaths.
+ */
+static List *
+find_saop_paths(PlannerInfo *root, RelOptInfo *rel,
+               List *clauses, List *outer_clauses,
+               bool istoplevel, bool isjoininner,
+               Relids outer_relids)
+{
+   bool        have_saop = false;
+   ListCell   *l;
+
+   /*
+    * Since find_usable_indexes is relatively expensive, don't bother to
+    * run it unless there are some top-level ScalarArrayOpExpr clauses.
+    */
+   foreach(l, clauses)
+   {
+       RestrictInfo *rinfo = (RestrictInfo *) lfirst(l);
+
+       Assert(IsA(rinfo, RestrictInfo));
+       if (IsA(rinfo->clause, ScalarArrayOpExpr))
+       {
+           have_saop = true;
+           break;
+       }
+   }
+   if (!have_saop)
+       return NIL;
+
+   return find_usable_indexes(root, rel,
+                              clauses, outer_clauses,
+                              istoplevel, isjoininner,
+                              outer_relids,
+                              SAOP_REQUIRE);
+}
+
+
 /*
  * generate_bitmap_or_paths
- *     Look through the list of clauses to find OR clauses and
- *     ScalarArrayOpExpr clauses, and generate a BitmapOrPath for each one
- *     we can handle that way.  Return a list of the generated BitmapOrPaths.
+ *     Look through the list of clauses to find OR clauses, and generate
+ *     a BitmapOrPath for each one we can handle that way.  Return a list
+ *     of the generated BitmapOrPaths.
  *
  * outer_clauses is a list of additional clauses that can be assumed true
  * for the purpose of generating indexquals, but are not to be searched for
@@ -416,7 +467,6 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
 {
    List       *result = NIL;
    List       *all_clauses;
-   bool        have_saop = false;
    ListCell   *l;
 
    /*
@@ -433,16 +483,9 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
        ListCell   *j;
 
        Assert(IsA(rinfo, RestrictInfo));
-       /*
-        * In this loop we ignore RestrictInfos that aren't ORs; but take
-        * note of ScalarArrayOpExpr for later.
-        */
+       /* Ignore RestrictInfos that aren't ORs */
        if (!restriction_is_or_clause(rinfo))
-       {
-           if (IsA(rinfo->clause, ScalarArrayOpExpr))
-               have_saop = true;
            continue;
-       }
 
        /*
         * We must be able to match at least one index to each of the arms of
@@ -516,29 +559,6 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
        }
    }
 
-   /*
-    * If we saw any top-level ScalarArrayOpExpr clauses, see if we can
-    * generate a bitmap index path that uses those but not any OR clauses.
-    */
-   if (have_saop)
-   {
-       List       *pathlist;
-       Path       *bitmapqual;
-
-       pathlist = find_usable_indexes(root, rel,
-                                      clauses,
-                                      outer_clauses,
-                                      false,
-                                      isjoininner,
-                                      outer_relids,
-                                      SAOP_REQUIRE);
-       if (pathlist != NIL)
-       {
-           bitmapqual = (Path *) create_bitmap_or_path(root, rel, pathlist);
-           result = lappend(result, bitmapqual);
-       }
-   }
-
    return result;
 }
 
@@ -1429,14 +1449,24 @@ best_inner_indexscan(PlannerInfo *root, RelOptInfo *rel,
                                     SAOP_FORBID);
 
    /*
-    * Generate BitmapOrPaths for any suitable OR-clauses or ScalarArrayOpExpr
-    * clauses present in the clause list.
+    * Generate BitmapOrPaths for any suitable OR-clauses present in the
+    * clause list.
     */
    bitindexpaths = generate_bitmap_or_paths(root, rel,
                                             clause_list, NIL,
                                             true,
                                             outer_relids);
 
+   /*
+    * Likewise, generate paths using ScalarArrayOpExpr clauses; these can't
+    * be simple indexscans but they can be used in bitmap scans.
+    */
+   bitindexpaths = list_concat(bitindexpaths,
+                               find_saop_paths(root, rel,
+                                               clause_list, NIL,
+                                               false, true,
+                                               outer_relids));
+
    /*
     * Include the regular index paths in bitindexpaths.
     */