Fix a bunch of bad interactions between partial indexes and the new

planning logic for bitmap indexscans.  Partial indexes create corner
cases in which a scan might be done with no explicit index qual conditions,
and the code wasn't handling those cases nicely.  Also be a little
tenser about eliminating redundant clauses in the generated plan.
Per report from Dmitry Karasik.

src/backend/optimizer/path/indxpath.c

@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/path/indxpath.c,v 1.186 2005/07/02 23:00:40 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/path/indxpath.c,v 1.187 2005/07/28 20:26:20 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -212,6 +212,8 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
  * subclause to any index on x alone, because such a Path would already have
  * been generated at the upper level.  So we could use an index on x,y,z
  * or an index on x,y for the OR subclause, but not an index on just x.
+ * When dealing with a partial index, a match of the index predicate to
+ * one of the "current" clauses also makes the index usable.
  *
  * If istoplevel is true (indicating we are considering the top level of a
  * rel's restriction clauses), we will include indexes in the result that
@@ -248,6 +250,8 @@ find_usable_indexes(PlannerInfo *root, RelOptInfo *rel,
 		List	   *restrictclauses;
 		List	   *index_pathkeys;
 		List	   *useful_pathkeys;
+		bool		useful_predicate;
+		bool		found_clause;
 		bool		index_is_ordered;
 
 		/*
@@ -258,29 +262,60 @@ find_usable_indexes(PlannerInfo *root, RelOptInfo *rel,
 		 * Otherwise, we have to test whether the added clauses are
 		 * sufficient to imply the predicate.  If so, we could use
 		 * the index in the current context.
+		 *
+		 * We set useful_predicate to true iff the predicate was proven
+		 * using the current set of clauses.  This is needed to prevent
+		 * matching a 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 (index->indpred != NIL && !index->predOK)
+		useful_predicate = false;
+		if (index->indpred != NIL)
 		{
-			if (istoplevel)
-				continue;		/* no point in trying to prove it */
+			if (index->predOK)
+			{
+				if (istoplevel)
+				{
+					/* we know predicate was proven from these clauses */
+					useful_predicate = true;
+				}
+			}
+			else
+			{
+				if (istoplevel)
+					continue;		/* no point in trying to prove it */
 
-			/* Form all_clauses if not done already */
-			if (all_clauses == NIL)
-				all_clauses = list_concat(list_copy(clauses),
-										  outer_clauses);
+				/* Form all_clauses if not done already */
+				if (all_clauses == NIL)
+					all_clauses = list_concat(list_copy(clauses),
+											  outer_clauses);
 
-			if (!predicate_implied_by(index->indpred, all_clauses) ||
-				predicate_implied_by(index->indpred, outer_clauses))
-				continue;
+				if (!predicate_implied_by(index->indpred, all_clauses))
+					continue;		/* can't use it at all */
+
+				if (!predicate_implied_by(index->indpred, outer_clauses))
+					useful_predicate = true;
+			}
 		}
 
 		/*
 		 * 1. Match the index against the available restriction clauses.
+		 * found_clause is set true only if at least one of the current
+		 * clauses was used.
 		 */
 		restrictclauses = group_clauses_by_indexkey(index,
 													clauses,
 													outer_clauses,
-													outer_relids);
+													outer_relids,
+													&found_clause);
+
+		/*
+		 * Not all index AMs support scans with no restriction clauses.
+		 * We can't generate a scan over an index with amoptionalkey = false
+		 * unless there's at least one restriction clause.
+		 */
+		if (restrictclauses == NIL && !index->amoptionalkey)
+			continue;
 
 		/*
 		 * 2. Compute pathkeys describing index's ordering, if any, then
@@ -300,20 +335,12 @@ find_usable_indexes(PlannerInfo *root, RelOptInfo *rel,
 
 		/*
 		 * 3. Generate an indexscan path if there are relevant restriction
-		 * clauses OR the index ordering is potentially useful for later
-		 * merging or final output ordering.
-		 *
-		 * If there is a predicate, consider it anyway since the index
-		 * predicate has already been found to match the query.  The
-		 * selectivity of the predicate might alone make the index useful.
-		 *
-		 * Note: not all index AMs support scans with no restriction clauses.
-		 * We can't generate a scan over an index with amoptionalkey = false
-		 * unless there's at least one restriction clause.
+		 * clauses in the current clauses, OR the index ordering is
+		 * potentially useful for later merging or final output ordering,
+		 * OR the index has a predicate that was proven by the current
+		 * clauses.
 		 */
-		if (restrictclauses != NIL ||
-			(index->amoptionalkey &&
-			 (useful_pathkeys != NIL || index->indpred != NIL)))
+		if (found_clause || useful_pathkeys != NIL || useful_predicate)
 		{
 			ipath = create_index_path(root, index,
 									  restrictclauses,
@@ -627,10 +654,9 @@ bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel, List *paths)
  * with one index key (in no particular order); the top list is ordered by
  * index key.  (This is depended on by expand_indexqual_conditions().)
  *
- * As explained in the comments for find_usable_indexes(), we can use
- * clauses from either of the given lists, but the result is required to
- * use at least one clause from the "current clauses" list.  We return
- * NIL if we don't find any such clause.
+ * We can use clauses from either the current clauses or outer_clauses lists,
+ * but *found_clause is set TRUE only if we used at least one clause from
+ * the "current clauses" list.  See find_usable_indexes() for motivation.
  *
  * outer_relids determines what Vars will be allowed on the other side
  * of a possible index qual; see match_clause_to_indexcol().
@@ -643,18 +669,25 @@ bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel, List *paths)
  * Example: given an index on (A,B,C), we would return ((C1 C2) () (C3 C4))
  * if we find that clauses C1 and C2 use column A, clauses C3 and C4 use
  * column C, and no clauses use column B.
+ *
+ * Note: in some circumstances we may find the same RestrictInfos coming
+ * from multiple places.  Defend against redundant outputs by using
+ * list_append_unique_ptr (pointer equality should be good enough).
  */
 List *
 group_clauses_by_indexkey(IndexOptInfo *index,
 						  List *clauses, List *outer_clauses,
-						  Relids outer_relids)
+						  Relids outer_relids,
+						  bool *found_clause)
 {
 	List	   *clausegroup_list = NIL;
-	bool		found_clause = false;
+	bool		found_outer_clause = false;
 	int			indexcol = 0;
 	Oid		   *classes = index->classlist;
 
-	if (clauses == NIL)
+	*found_clause = false;		/* default result */
+
+	if (clauses == NIL && outer_clauses == NIL)
 		return NIL;				/* cannot succeed */
 
 	do
@@ -675,8 +708,8 @@ group_clauses_by_indexkey(IndexOptInfo *index,
 										 rinfo,
 										 outer_relids))
 			{
-				clausegroup = lappend(clausegroup, rinfo);
-				found_clause = true;
+				clausegroup = list_append_unique_ptr(clausegroup, rinfo);
+				*found_clause = true;
 			}
 		}
 
@@ -691,7 +724,10 @@ group_clauses_by_indexkey(IndexOptInfo *index,
 										 curClass,
 										 rinfo,
 										 outer_relids))
-				clausegroup = lappend(clausegroup, rinfo);
+			{
+				clausegroup = list_append_unique_ptr(clausegroup, rinfo);
+				found_outer_clause = true;
+			}
 		}
 
 		/*
@@ -707,8 +743,8 @@ group_clauses_by_indexkey(IndexOptInfo *index,
 
 	} while (!DoneMatchingIndexKeys(classes));
 
-	if (!found_clause)
-		return NIL;
+	if (!*found_clause && !found_outer_clause)
+		return NIL;				/* no indexable clauses anywhere */
 
 	return clausegroup_list;
 }

src/backend/optimizer/path/orindxpath.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/path/orindxpath.c,v 1.73 2005/07/02 23:00:40 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/path/orindxpath.c,v 1.74 2005/07/28 20:26:20 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -134,14 +134,24 @@ create_or_index_quals(PlannerInfo *root, RelOptInfo *rel)
 		return false;
 
 	/*
-	 * Convert the path's indexclauses structure to a RestrictInfo tree,
-	 * and add it to the rel's restriction list.
+	 * Convert the path's indexclauses structure to a RestrictInfo tree.
+	 * We include any partial-index predicates so as to get a reasonable
+	 * representation of what the path is actually scanning.
 	 */
-	newrinfos = make_restrictinfo_from_bitmapqual((Path *) bestpath, true);
-	Assert(list_length(newrinfos) == 1);
+	newrinfos = make_restrictinfo_from_bitmapqual((Path *) bestpath,
+												  true, true);
+
+	/* It's possible we get back something other than a single OR clause */
+	if (list_length(newrinfos) != 1)
+		return false;
 	or_rinfo = (RestrictInfo *) linitial(newrinfos);
 	Assert(IsA(or_rinfo, RestrictInfo));
+	if (!restriction_is_or_clause(or_rinfo))
+		return false;
 
+	/*
+	 * OK, add it to the rel's restriction list.
+	 */
 	rel->baserestrictinfo = list_concat(rel->baserestrictinfo, newrinfos);
 
 	/*

src/backend/optimizer/plan/createplan.c

@@ -10,7 +10,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/plan/createplan.c,v 1.195 2005/07/23 21:05:46 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/plan/createplan.c,v 1.196 2005/07/28 20:26:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -900,7 +900,7 @@ create_bitmap_scan_plan(PlannerInfo *root,
 	 */
 	if (best_path->isjoininner)
 	{
-		scan_clauses = list_union(scan_clauses, bitmapqualorig);
+		scan_clauses = list_concat_unique(scan_clauses, bitmapqualorig);
 	}
 
 	/*
@@ -962,6 +962,9 @@ create_bitmap_scan_plan(PlannerInfo *root,
  * (in implicit-AND form, without RestrictInfos) describing the original index
  * conditions and the generated indexqual conditions.  The latter is made to
  * exclude lossy index operators.
+ *
+ * Note: if you find yourself changing this, you probably need to change
+ * make_restrictinfo_from_bitmapqual too.
  */
 static Plan *
 create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual,
@@ -977,6 +980,13 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual,
 		List	   *subindexquals = NIL;
 		ListCell   *l;
 
+		/*
+		 * There may well be redundant quals among the subplans, since a
+		 * top-level WHERE qual might have gotten used to form several
+		 * different index quals.  We don't try exceedingly hard to
+		 * eliminate redundancies, but we do eliminate obvious duplicates
+		 * by using list_concat_unique.
+		 */
 		foreach(l, apath->bitmapquals)
 		{
 			Plan   *subplan;
@@ -986,8 +996,8 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual,
 			subplan = create_bitmap_subplan(root, (Path *) lfirst(l),
 											&subqual, &subindexqual);
 			subplans = lappend(subplans, subplan);
-			subquals = list_concat(subquals, subqual);
-			subindexquals = list_concat(subindexquals, subindexqual);
+			subquals = list_concat_unique(subquals, subqual);
+			subindexquals = list_concat_unique(subindexquals, subindexqual);
 		}
 		plan = (Plan *) make_bitmap_and(subplans);
 		plan->startup_cost = apath->path.startup_cost;
@@ -1004,8 +1014,15 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual,
 		List	   *subplans = NIL;
 		List	   *subquals = NIL;
 		List	   *subindexquals = NIL;
+		bool		const_true_subqual = false;
+		bool		const_true_subindexqual = false;
 		ListCell   *l;
 
+		/*
+		 * Here, we detect both obvious redundancies and qual-free subplans.
+		 * A qual-free subplan would cause us to generate "... OR true ..."
+		 * which we may as well reduce to just "true".
+		 */
 		foreach(l, opath->bitmapquals)
 		{
 			Plan   *subplan;
@@ -1015,10 +1032,16 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual,
 			subplan = create_bitmap_subplan(root, (Path *) lfirst(l),
 											&subqual, &subindexqual);
 			subplans = lappend(subplans, subplan);
-			subquals = lappend(subquals,
-							   make_ands_explicit(subqual));
-			subindexquals = lappend(subindexquals,
-									make_ands_explicit(subindexqual));
+			if (subqual == NIL)
+				const_true_subqual = true;
+			else if (!const_true_subqual)
+				subquals = list_append_unique(subquals,
+											  make_ands_explicit(subqual));
+			if (subindexqual == NIL)
+				const_true_subindexqual = true;
+			else if (!const_true_subindexqual)
+				subindexquals = list_append_unique(subindexquals,
+												   make_ands_explicit(subindexqual));
 		}
 		plan = (Plan *) make_bitmap_or(subplans);
 		plan->startup_cost = opath->path.startup_cost;
@@ -1026,8 +1049,23 @@ create_bitmap_subplan(PlannerInfo *root, Path *bitmapqual,
 		plan->plan_rows =
 			clamp_row_est(opath->bitmapselectivity * opath->path.parent->tuples);
 		plan->plan_width = 0;	/* meaningless */
-		*qual = list_make1(make_orclause(subquals));
-		*indexqual = list_make1(make_orclause(subindexquals));
+		/*
+		 * If there were constant-TRUE subquals, the OR reduces to constant
+		 * TRUE.  Also, avoid generating one-element ORs, which could happen
+		 * due to redundancy elimination.
+		 */
+		if (const_true_subqual)
+			*qual = NIL;
+		else if (list_length(subquals) <= 1)
+			*qual = subquals;
+		else
+			*qual = list_make1(make_orclause(subquals));
+		if (const_true_subindexqual)
+			*indexqual = NIL;
+		else if (list_length(subindexquals) <= 1)
+			*indexqual = subindexquals;
+		else
+			*indexqual = list_make1(make_orclause(subindexquals));
 	}
 	else if (IsA(bitmapqual, IndexPath))
 	{
@@ -1204,6 +1242,14 @@ create_nestloop_plan(PlannerInfo *root,
 	{
 		/*
 		 * Same deal for bitmapped index scans.
+		 *
+		 * Note: both here and above, we ignore any implicit index restrictions
+		 * associated with the use of partial indexes.  This is OK because
+		 * we're only trying to prove we can dispense with some join quals;
+		 * failing to prove that doesn't result in an incorrect plan.  It is
+		 * the right way to proceed because adding more quals to the stuff
+		 * we got from the original query would just make it harder to detect
+		 * duplication.
 		 */
 		BitmapHeapPath *innerpath = (BitmapHeapPath *) best_path->innerjoinpath;
 
@@ -1212,7 +1258,9 @@ create_nestloop_plan(PlannerInfo *root,
 			List	   *bitmapclauses;
 
 			bitmapclauses =
-				make_restrictinfo_from_bitmapqual(innerpath->bitmapqual, true);
+				make_restrictinfo_from_bitmapqual(innerpath->bitmapqual,
+												  true,
+												  false);
 			joinrestrictclauses =
 				select_nonredundant_join_clauses(root,
 												 joinrestrictclauses,

src/backend/optimizer/plan/planagg.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/plan/planagg.c,v 1.6 2005/07/23 21:05:46 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/plan/planagg.c,v 1.7 2005/07/28 20:26:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -313,6 +313,7 @@ build_minmax_path(PlannerInfo *root, RelOptInfo *rel, MinMaxAggInfo *info)
 		List	   *restrictclauses;
 		IndexPath  *new_path;
 		Cost		new_cost;
+		bool		found_clause;
 
 		/* Ignore non-btree indexes */
 		if (index->relam != BTREE_AM_OID)
@@ -346,7 +347,8 @@ build_minmax_path(PlannerInfo *root, RelOptInfo *rel, MinMaxAggInfo *info)
 		restrictclauses = group_clauses_by_indexkey(index,
 													index->rel->baserestrictinfo,
 													NIL,
-													NULL);
+													NULL,
+													&found_clause);
 
 		if (list_length(restrictclauses) < indexcol)
 			continue;			/* definitely haven't got enough */

src/backend/optimizer/util/restrictinfo.c

@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/util/restrictinfo.c,v 1.38 2005/07/02 23:00:41 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/util/restrictinfo.c,v 1.39 2005/07/28 20:26:21 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -17,6 +17,7 @@
 #include "optimizer/clauses.h"
 #include "optimizer/cost.h"
 #include "optimizer/paths.h"
+#include "optimizer/predtest.h"
 #include "optimizer/restrictinfo.h"
 #include "optimizer/var.h"
 
@@ -70,31 +71,49 @@ make_restrictinfo(Expr *clause, bool is_pushed_down, Relids required_relids)
  * RestrictInfo node(s) equivalent to the condition represented by the
  * indexclauses of the Path structure.
  *
- * The result is a List since we might need to return multiple RestrictInfos.
+ * The result is a List (effectively, implicit-AND representation) of
+ * RestrictInfos.
+ *
+ * If include_predicates is true, we add any partial index predicates to
+ * the explicit index quals.  When this is not true, we return a condition
+ * that might be weaker than the actual scan represents.
  *
  * To do this through the normal make_restrictinfo() API, callers would have
  * to strip off the RestrictInfo nodes present in the indexclauses lists, and
  * then make_restrictinfo() would have to build new ones.  It's better to have
  * a specialized routine to allow sharing of RestrictInfos.
+ *
+ * The qual manipulations here are much the same as in create_bitmap_subplan;
+ * keep the two routines in sync!
  */
 List *
-make_restrictinfo_from_bitmapqual(Path *bitmapqual, bool is_pushed_down)
+make_restrictinfo_from_bitmapqual(Path *bitmapqual,
+								  bool is_pushed_down,
+								  bool include_predicates)
 {
 	List	   *result;
+	ListCell   *l;
 
 	if (IsA(bitmapqual, BitmapAndPath))
 	{
 		BitmapAndPath *apath = (BitmapAndPath *) bitmapqual;
-		ListCell   *l;
 
+		/*
+		 * There may well be redundant quals among the subplans, since a
+		 * top-level WHERE qual might have gotten used to form several
+		 * different index quals.  We don't try exceedingly hard to
+		 * eliminate redundancies, but we do eliminate obvious duplicates
+		 * by using list_concat_unique.
+		 */
 		result = NIL;
 		foreach(l, apath->bitmapquals)
 		{
 			List	   *sublist;
 
 			sublist = make_restrictinfo_from_bitmapqual((Path *) lfirst(l),
-														is_pushed_down);
-			result = list_concat(result, sublist);
+														is_pushed_down,
+														include_predicates);
+			result = list_concat_unique(result, sublist);
 		}
 	}
 	else if (IsA(bitmapqual, BitmapOrPath))
@@ -102,38 +121,77 @@ make_restrictinfo_from_bitmapqual(Path *bitmapqual, bool is_pushed_down)
 		BitmapOrPath *opath = (BitmapOrPath *) bitmapqual;
 		List	   *withris = NIL;
 		List	   *withoutris = NIL;
-		ListCell   *l;
 
+		/*
+		 * Here, we detect both obvious redundancies and qual-free subplans.
+		 * A qual-free subplan would cause us to generate "... OR true ..."
+		 * which we may as well reduce to just "true".
+		 */
 		foreach(l, opath->bitmapquals)
 		{
 			List	   *sublist;
 
 			sublist = make_restrictinfo_from_bitmapqual((Path *) lfirst(l),
-														is_pushed_down);
+														is_pushed_down,
+														include_predicates);
 			if (sublist == NIL)
 			{
-				/* constant TRUE input yields constant TRUE OR result */
-				/* (though this probably cannot happen) */
+				/*
+				 * If we find a qual-less subscan, it represents a constant
+				 * TRUE, and hence the OR result is also constant TRUE, so
+				 * we can stop here.
+				 */
 				return NIL;
 			}
 			/* Create AND subclause with RestrictInfos */
-			withris = lappend(withris, make_ands_explicit(sublist));
+			withris = list_append_unique(withris,
+										 make_ands_explicit(sublist));
 			/* And one without */
 			sublist = get_actual_clauses(sublist);
-			withoutris = lappend(withoutris, make_ands_explicit(sublist));
+			withoutris = list_append_unique(withoutris,
+											make_ands_explicit(sublist));
+		}
+
+		/*
+		 * Avoid generating one-element ORs, which could happen
+		 * due to redundancy elimination.
+		 */
+		if (list_length(withris) <= 1)
+			result = withris;
+		else
+		{
+			/* Here's the magic part not available to outside callers */
+			result =
+				list_make1(make_restrictinfo_internal(make_orclause(withoutris),
+													  make_orclause(withris),
+													  is_pushed_down,
+													  NULL));
 		}
-		/* Here's the magic part not available to outside callers */
-		result =
-			list_make1(make_restrictinfo_internal(make_orclause(withoutris),
-												  make_orclause(withris),
-												  is_pushed_down,
-												  NULL));
 	}
 	else if (IsA(bitmapqual, IndexPath))
 	{
 		IndexPath *ipath = (IndexPath *) bitmapqual;
 
 		result = list_copy(ipath->indexclauses);
+		if (include_predicates && ipath->indexinfo->indpred != NIL)
+		{
+			foreach(l, ipath->indexinfo->indpred)
+			{
+				Expr   *pred = (Expr *) lfirst(l);
+
+				/*
+				 * We know that the index predicate must have been implied
+				 * by the query condition as a whole, but it may or may not
+				 * be implied by the conditions that got pushed into the
+				 * bitmapqual.  Avoid generating redundant conditions.
+				 */
+				if (!predicate_implied_by(list_make1(pred), result))
+					result = lappend(result,
+									 make_restrictinfo(pred,
+													   is_pushed_down,
+													   NULL));
+			}
+		}
 	}
 	else
 	{