Rethink the order of expression preprocessing: eval_const_expressions

really ought to run before canonicalize_qual, because it can now produce
forms that canonicalize_qual knows how to improve (eg, NOT clauses).
Also, because eval_const_expressions already knows about flattening
nested ANDs and ORs into N-argument form, the initial flatten_andors
pass in canonicalize_qual is now completely redundant and can be
removed.  This doesn't save a whole lot of code, but the time and
palloc traffic eliminated is a useful gain on large expression trees.

7 files changed, 61 insertions, 140 deletions

diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 324dad87c73..e1328c0f9b8 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -9,7 +9,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/path/indxpath.c,v 1.171 2005/03/27 06:29:36 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/path/indxpath.c,v 1.172 2005/03/28 00:58:22 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -750,8 +750,8 @@ check_partial_indexes(Query *root, RelOptInfo *rel)
  *	  that the given predicate is true.
  *
  *	  The top-level List structure of each list corresponds to an AND list.
- *	  We assume that canonicalize_qual() has been applied and so there are
- *	  no un-flattened ANDs or ORs (e.g., no AND immediately within an AND,
+ *	  We assume that eval_const_expressions() has been applied and so there
+ *	  are no un-flattened ANDs or ORs (e.g., no AND immediately within an AND,
  *	  including AND just below the top-level List structure).
  *	  If this is not true we might fail to prove an implication that is
  *	  valid, but no worse consequences will ensue.
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 969f3711389..5f3c7510cdd 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/plan/planner.c,v 1.180 2005/03/17 23:44:26 neilc Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/plan/planner.c,v 1.181 2005/03/28 00:58:23 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -422,13 +422,17 @@ preprocess_expression(Query *parse, Node *expr, int kind)
 		expr = flatten_join_alias_vars(parse, expr);
 
 	/*
-	 * If it's a qual or havingQual, canonicalize it.  It seems most
-	 * useful to do this before applying eval_const_expressions, since the
-	 * latter can optimize flattened AND/ORs better than unflattened ones.
+	 * Simplify constant expressions.
 	 *
-	 * Note: all processing of a qual expression after this point must be
-	 * careful to maintain AND/OR flatness --- that is, do not generate a
-	 * tree with AND directly under AND, nor OR directly under OR.
+	 * Note: this also flattens nested AND and OR expressions into N-argument
+	 * form.  All processing of a qual expression after this point must be
+	 * careful to maintain AND/OR flatness --- that is, do not generate a tree
+	 * with AND directly under AND, nor OR directly under OR.
+	 */
+	expr = eval_const_expressions(expr);
+
+	/*
+	 * If it's a qual or havingQual, canonicalize it.
 	 */
 	if (kind == EXPRKIND_QUAL)
 	{
@@ -440,11 +444,6 @@ preprocess_expression(Query *parse, Node *expr, int kind)
 #endif
 	}
 
-	/*
-	 * Simplify constant expressions.
-	 */
-	expr = eval_const_expressions(expr);
-
 	/* Expand SubLinks to SubPlans */
 	if (parse->hasSubLinks)
 		expr = SS_process_sublinks(expr, (kind == EXPRKIND_QUAL));
diff --git a/src/backend/optimizer/prep/prepqual.c b/src/backend/optimizer/prep/prepqual.c
index 8d92ee317a0..c153d312fa6 100644
--- a/src/backend/optimizer/prep/prepqual.c
+++ b/src/backend/optimizer/prep/prepqual.c
@@ -3,12 +3,29 @@
  * prepqual.c
  *	  Routines for preprocessing qualification expressions
  *
+ *
+ * The parser regards AND and OR as purely binary operators, so a qual like
+ *		(A = 1) OR (A = 2) OR (A = 3) ...
+ * will produce a nested parsetree
+ *		(OR (A = 1) (OR (A = 2) (OR (A = 3) ...)))
+ * In reality, the optimizer and executor regard AND and OR as N-argument
+ * operators, so this tree can be flattened to
+ *		(OR (A = 1) (A = 2) (A = 3) ...)
+ *
+ * Formerly, this module was responsible for doing the initial flattening,
+ * but now we leave it to eval_const_expressions to do that since it has to
+ * make a complete pass over the expression tree anyway.  Instead, we just
+ * have to ensure that our manipulations preserve AND/OR flatness.
+ * pull_ands() and pull_ors() are used to maintain flatness of the AND/OR
+ * tree after local transformations that might introduce nested AND/ORs.
+ *
+ *
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/prep/prepqual.c,v 1.48 2004/12/31 22:00:20 pgsql Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/prep/prepqual.c,v 1.49 2005/03/28 00:58:23 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -21,7 +38,6 @@
 #include "utils/lsyscache.h"
 
 
-static Node *flatten_andors_mutator(Node *node, void *context);
 static List *pull_ands(List *andlist);
 static List *pull_ors(List *orlist);
 static Expr *find_nots(Expr *qual);
@@ -40,6 +56,11 @@ static Expr *process_duplicate_ors(List *orlist);
  * actual usefulness, and so now the transformation doesn't involve any
  * notion of reaching a canonical form.
  *
+ * NOTE: we assume the input has already been through eval_const_expressions
+ * and therefore possesses AND/OR flatness.  Formerly this function included
+ * its own flattening logic, but that requires a useless extra pass over the
+ * tree.
+ *
  * Returns the modified qualification.
  */
 Expr *
@@ -52,17 +73,12 @@ canonicalize_qual(Expr *qual)
 		return NULL;
 
 	/*
-	 * Flatten AND and OR groups throughout the expression tree.
-	 */
-	newqual = (Expr *) flatten_andors((Node *) qual);
-
-	/*
 	 * Push down NOTs.	We do this only in the top-level boolean
 	 * expression, without examining arguments of operators/functions. The
 	 * main reason for doing this is to expose as much top-level AND/OR
 	 * structure as we can, so there's no point in descending further.
 	 */
-	newqual = find_nots(newqual);
+	newqual = find_nots(qual);
 
 	/*
 	 * Pull up redundant subclauses in OR-of-AND trees.  Again, we do this
@@ -74,101 +90,6 @@ canonicalize_qual(Expr *qual)
 }
 
 
-/*--------------------
- * The parser regards AND and OR as purely binary operators, so a qual like
- *		(A = 1) OR (A = 2) OR (A = 3) ...
- * will produce a nested parsetree
- *		(OR (A = 1) (OR (A = 2) (OR (A = 3) ...)))
- * In reality, the optimizer and executor regard AND and OR as n-argument
- * operators, so this tree can be flattened to
- *		(OR (A = 1) (A = 2) (A = 3) ...)
- * which is the responsibility of the routines below.
- *
- * flatten_andors() does the basic transformation with no initial assumptions.
- * pull_ands() and pull_ors() are used to maintain flatness of the AND/OR
- * tree after local transformations that might introduce nested AND/ORs.
- *--------------------
- */
-
-/*
- * flatten_andors
- *	  Given an expression tree, simplify nested AND/OR clauses into flat
- *	  AND/OR clauses with more arguments.  The entire tree is processed.
- *
- * Returns the rebuilt expr (note original structure is not touched).
- *
- * This is exported so that other modules can perform the part of
- * canonicalize_qual processing that applies to entire trees, rather
- * than just the top-level boolean expressions.
- */
-Node *
-flatten_andors(Node *node)
-{
-	return flatten_andors_mutator(node, NULL);
-}
-
-static Node *
-flatten_andors_mutator(Node *node, void *context)
-{
-	if (node == NULL)
-		return NULL;
-	if (IsA(node, BoolExpr))
-	{
-		BoolExpr   *bexpr = (BoolExpr *) node;
-
-		if (bexpr->boolop == AND_EXPR)
-		{
-			List	   *out_list = NIL;
-			ListCell   *arg;
-
-			foreach(arg, bexpr->args)
-			{
-				Node	   *subexpr = flatten_andors((Node *) lfirst(arg));
-
-				/*
-				 * Note: we can destructively concat the subexpression's
-				 * arglist because we know the recursive invocation of
-				 * flatten_andors will have built a new arglist not shared
-				 * with any other expr. Otherwise we'd need a list_copy
-				 * here.
-				 */
-				if (and_clause(subexpr))
-					out_list = list_concat(out_list,
-										   ((BoolExpr *) subexpr)->args);
-				else
-					out_list = lappend(out_list, subexpr);
-			}
-			return (Node *) make_andclause(out_list);
-		}
-		if (bexpr->boolop == OR_EXPR)
-		{
-			List	   *out_list = NIL;
-			ListCell   *arg;
-
-			foreach(arg, bexpr->args)
-			{
-				Node	   *subexpr = flatten_andors((Node *) lfirst(arg));
-
-				/*
-				 * Note: we can destructively concat the subexpression's
-				 * arglist because we know the recursive invocation of
-				 * flatten_andors will have built a new arglist not shared
-				 * with any other expr. Otherwise we'd need a list_copy
-				 * here.
-				 */
-				if (or_clause(subexpr))
-					out_list = list_concat(out_list,
-										   ((BoolExpr *) subexpr)->args);
-				else
-					out_list = lappend(out_list, subexpr);
-			}
-			return (Node *) make_orclause(out_list);
-		}
-		/* else it's a NOT clause, fall through */
-	}
-	return expression_tree_mutator(node, flatten_andors_mutator, context);
-}
-
 /*
  * pull_ands
  *	  Recursively flatten nested AND clauses into a single and-clause list.
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index cf585cccbc8..15418d4e957 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.189 2005/03/27 19:18:02 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/util/clauses.c,v 1.190 2005/03/28 00:58:24 tgl Exp $
  *
  * HISTORY
  *	  AUTHOR			DATE			MAJOR EVENT
@@ -1190,6 +1190,9 @@ rowtype_field_matches(Oid rowtypeid, int fieldnum,
  *
  * We assume that the tree has already been type-checked and contains
  * only operators and functions that are reasonable to try to execute.
+ *
+ * NOTE: the planner assumes that this will always flatten nested AND and
+ * OR clauses into N-argument form.  See comments in prepqual.c.
  *--------------------
  */
 Node *
@@ -1871,7 +1874,7 @@ eval_const_expressions_mutator(Node *node,
  * is TRUE and at least one is NULL.
  *
  * This is split out as a subroutine so that we can recurse to fold sub-ORs
- * into the upper OR clause, thereby preserving AND/OR flatness.
+ * into the upper OR clause, thereby ensuring that nested ORs are flattened.
  *
  * The output arguments *haveNull and *forceTrue must be initialized FALSE
  * by the caller.  They will be set TRUE if a null constant or true constant,
@@ -1931,7 +1934,7 @@ simplify_or_arguments(List *args, bool *haveNull, bool *forceTrue)
  * is FALSE and at least one is NULL.
  *
  * This is split out as a subroutine so that we can recurse to fold sub-ANDs
- * into the upper AND clause, thereby preserving AND/OR flatness.
+ * into the upper AND clause, thereby ensuring that nested ANDs are flattened.
  *
  * The output arguments *haveNull and *forceFalse must be initialized FALSE
  * by the caller.  They will be set TRUE if a null constant or false constant,
diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c
index 20590a98e0e..3dad1a16072 100644
--- a/src/backend/optimizer/util/restrictinfo.c
+++ b/src/backend/optimizer/util/restrictinfo.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/optimizer/util/restrictinfo.c,v 1.31 2004/12/31 22:00:23 pgsql Exp $
+ *	  $PostgreSQL: pgsql/src/backend/optimizer/util/restrictinfo.c,v 1.32 2005/03/28 00:58:24 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -63,7 +63,7 @@ make_restrictinfo(Expr *clause, bool is_pushed_down, bool valid_everywhere)
 	}
 	else
 	{
-		/* Shouldn't be an AND clause, else flatten_andors messed up */
+		/* Shouldn't be an AND clause, else AND/OR flattening messed up */
 		Assert(!and_clause((Node *) clause));
 
 		orclause = NULL;
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index d1143381d2d..a6660fc4d32 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *	  $PostgreSQL: pgsql/src/backend/utils/cache/relcache.c,v 1.216 2005/03/07 04:42:16 tgl Exp $
+ *	  $PostgreSQL: pgsql/src/backend/utils/cache/relcache.c,v 1.217 2005/03/28 00:58:26 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -2788,14 +2788,11 @@ RelationGetIndexExpressions(Relation relation)
 	pfree(exprsString);
 
 	/*
-	 * Run the expressions through flatten_andors and
-	 * eval_const_expressions. This is not just an optimization, but is
-	 * necessary, because the planner will be comparing them to
-	 * similarly-processed qual clauses, and may fail to detect valid
-	 * matches without this.
+	 * Run the expressions through eval_const_expressions. This is not just an
+	 * optimization, but is necessary, because the planner will be comparing
+	 * them to similarly-processed qual clauses, and may fail to detect valid
+	 * matches without this.  We don't bother with canonicalize_qual, however.
 	 */
-	result = (List *) flatten_andors((Node *) result);
-
 	result = (List *) eval_const_expressions((Node *) result);
 
 	/*
@@ -2863,16 +2860,18 @@ RelationGetIndexPredicate(Relation relation)
 	pfree(predString);
 
 	/*
-	 * Run the expression through canonicalize_qual and
-	 * eval_const_expressions. This is not just an optimization, but is
-	 * necessary, because the planner will be comparing it to
-	 * similarly-processed qual clauses, and may fail to detect valid
-	 * matches without this.
+	 * Run the expression through const-simplification and canonicalization.
+	 * This is not just an optimization, but is necessary, because the planner
+	 * will be comparing it to similarly-processed qual clauses, and may fail
+	 * to detect valid matches without this.  This must match the processing
+	 * done to qual clauses in preprocess_expression()!  (We can skip the
+	 * stuff involving subqueries, however, since we don't allow any in
+	 * index predicates.)
 	 */
-	result = (List *) canonicalize_qual((Expr *) result);
-
 	result = (List *) eval_const_expressions((Node *) result);
 
+	result = (List *) canonicalize_qual((Expr *) result);
+
 	/*
 	 * Also mark any coercion format fields as "don't care", so that the
 	 * planner can match to both explicit and implicit coercions.
diff --git a/src/include/optimizer/prep.h b/src/include/optimizer/prep.h
index 223c86eb1e5..6a86b2e174f 100644
--- a/src/include/optimizer/prep.h
+++ b/src/include/optimizer/prep.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2005, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $PostgreSQL: pgsql/src/include/optimizer/prep.h,v 1.48 2005/03/17 23:45:09 neilc Exp $
+ * $PostgreSQL: pgsql/src/include/optimizer/prep.h,v 1.49 2005/03/28 00:58:26 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -37,7 +37,6 @@ extern Relids get_relids_for_join(Query *parse, int joinrelid);
  * prototypes for prepqual.c
  */
 extern Expr *canonicalize_qual(Expr *qual);
-extern Node *flatten_andors(Node *node);
 
 /*
  * prototypes for preptlist.c