Refactor pull_var_clause's API to make it less tedious to extend.

tglsfdc · tglsfdc · commit 364a9f47ab36 · 2016-03-10T15:53:07.000-05:00
In commit 1d97c19 and later c1d9579, we extended pull_var_clause's API by adding enum-type arguments. That's sort of a pain to maintain, though, because it means every time we add a new behavior we must touch every last one of the call sites, even if there's a reasonable default behavior that most of them could use. Let's switch over to using a bitmask of flags, instead; that seems more maintainable and might save a nanosecond or two as well. This commit changes no behavior in itself, though I'm going to follow it up with one that does add a new behavior. In passing, remove flatten_tlist(), which has not been used since 9.1 and would otherwise need the same API changes. Removing these enums means that optimizer/tlist.h no longer needs to depend on optimizer/var.h. Changing that caused a number of C files to need addition of #include "optimizer/var.h" (probably we can thank old runs of pgrminclude for that); but on balance it seems like a good change anyway.
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
@@ -734,7 +734,6 @@ build_tlist_to_deparse(RelOptInfo *foreignrel)
 	tlist = add_to_flat_tlist(tlist, foreignrel->reltarget.exprs);
 	tlist = add_to_flat_tlist(tlist,
 							  pull_var_clause((Node *) fpinfo->local_conds,
-											  PVC_REJECT_AGGREGATES,
 											  PVC_RECURSE_PLACEHOLDERS));
 
 	return tlist;
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
@@ -2006,9 +2006,7 @@ StoreRelCheck(Relation rel, char *ccname, Node *expr,
 	 * in check constraints; it would fail to examine the contents of
 	 * subselects.
 	 */
-	varList = pull_var_clause(expr,
-							  PVC_REJECT_AGGREGATES,
-							  PVC_REJECT_PLACEHOLDERS);
+	varList = pull_var_clause(expr, 0);
 	keycount = list_length(varList);
 
 	if (keycount > 0)
@@ -2323,9 +2321,7 @@ AddRelationNewConstraints(Relation rel,
 			List	   *vars;
 			char	   *colname;
 
-			vars = pull_var_clause(expr,
-								   PVC_REJECT_AGGREGATES,
-								   PVC_REJECT_PLACEHOLDERS);
+			vars = pull_var_clause(expr, 0);
 
 			/* eliminate duplicates */
 			vars = list_union(NIL, vars);
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
@@ -352,9 +352,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
 		 * subselects in WHEN clauses; it would fail to examine the contents
 		 * of subselects.
 		 */
-		varList = pull_var_clause(whenClause,
-								  PVC_REJECT_AGGREGATES,
-								  PVC_REJECT_PLACEHOLDERS);
+		varList = pull_var_clause(whenClause, 0);
 		foreach(lc, varList)
 		{
 			Var		   *var = (Var *) lfirst(lc);
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
@@ -2509,9 +2509,7 @@ qual_is_pushdown_safe(Query *subquery, Index rti, Node *qual,
 	 * Examine all Vars used in clause; since it's a restriction clause, all
 	 * such Vars must refer to subselect output columns.
 	 */
-	vars = pull_var_clause(qual,
-						   PVC_REJECT_AGGREGATES,
-						   PVC_INCLUDE_PLACEHOLDERS);
+	vars = pull_var_clause(qual, PVC_INCLUDE_PLACEHOLDERS);
 	foreach(vl, vars)
 	{
 		Var		   *var = (Var *) lfirst(vl);
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
@@ -910,7 +910,7 @@ generate_base_implied_equalities_no_const(PlannerInfo *root,
 	{
 		EquivalenceMember *cur_em = (EquivalenceMember *) lfirst(lc);
 		List	   *vars = pull_var_clause((Node *) cur_em->em_expr,
-										   PVC_RECURSE_AGGREGATES,
+										   PVC_RECURSE_AGGREGATES |
 										   PVC_INCLUDE_PLACEHOLDERS);
 
 		add_vars_to_targetlist(root, vars, ec->ec_relids, false);
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
@@ -29,6 +29,7 @@
 #include "optimizer/paths.h"
 #include "optimizer/planmain.h"
 #include "optimizer/tlist.h"
+#include "optimizer/var.h"
 #include "utils/lsyscache.h"
 
 /* local functions */
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
@@ -5335,7 +5335,7 @@ prepare_sort_from_pathkeys(Plan *lefttree, List *pathkeys,
 
 				sortexpr = em->em_expr;
 				exprvars = pull_var_clause((Node *) sortexpr,
-										   PVC_INCLUDE_AGGREGATES,
+										   PVC_INCLUDE_AGGREGATES |
 										   PVC_INCLUDE_PLACEHOLDERS);
 				foreach(k, exprvars)
 				{
diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c
@@ -146,7 +146,7 @@ void
 build_base_rel_tlists(PlannerInfo *root, List *final_tlist)
 {
 	List	   *tlist_vars = pull_var_clause((Node *) final_tlist,
-											 PVC_RECURSE_AGGREGATES,
+											 PVC_RECURSE_AGGREGATES |
 											 PVC_INCLUDE_PLACEHOLDERS);
 
 	if (tlist_vars != NIL)
@@ -161,7 +161,7 @@ build_base_rel_tlists(PlannerInfo *root, List *final_tlist)
 	if (root->parse->havingQual)
 	{
 		List	   *having_vars = pull_var_clause(root->parse->havingQual,
-												  PVC_RECURSE_AGGREGATES,
+												  PVC_RECURSE_AGGREGATES |
 												  PVC_INCLUDE_PLACEHOLDERS);
 
 		if (having_vars != NIL)
@@ -1787,7 +1787,7 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
 	if (bms_membership(relids) == BMS_MULTIPLE)
 	{
 		List	   *vars = pull_var_clause(clause,
-										   PVC_RECURSE_AGGREGATES,
+										   PVC_RECURSE_AGGREGATES |
 										   PVC_INCLUDE_PLACEHOLDERS);
 
 		add_vars_to_targetlist(root, vars, relids, false);
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
@@ -43,6 +43,7 @@
 #include "optimizer/prep.h"
 #include "optimizer/subselect.h"
 #include "optimizer/tlist.h"
+#include "optimizer/var.h"
 #include "parser/analyze.h"
 #include "parser/parsetree.h"
 #include "parser/parse_agg.h"
@@ -3840,7 +3841,7 @@ make_group_input_target(PlannerInfo *root, List *tlist)
 	 * pulled out here, too.
 	 */
 	non_group_vars = pull_var_clause((Node *) non_group_cols,
-									 PVC_RECURSE_AGGREGATES,
+									 PVC_RECURSE_AGGREGATES |
 									 PVC_INCLUDE_PLACEHOLDERS);
 	sub_tlist = add_to_flat_tlist(sub_tlist, non_group_vars);
 
@@ -4088,7 +4089,7 @@ make_window_input_target(PlannerInfo *root,
 	 * at higher levels.
 	 */
 	flattenable_vars = pull_var_clause((Node *) flattenable_cols,
-									   PVC_INCLUDE_AGGREGATES,
+									   PVC_INCLUDE_AGGREGATES |
 									   PVC_INCLUDE_PLACEHOLDERS);
 	new_tlist = add_to_flat_tlist(new_tlist, flattenable_vars);
 
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
@@ -1641,12 +1641,12 @@ set_join_references(PlannerInfo *root, Join *join, int rtoffset)
  *
  * In most cases, we have to match up individual Vars in the tlist and
  * qual expressions with elements of the subplan's tlist (which was
- * generated by flatten_tlist() from these selfsame expressions, so it
- * should have all the required variables).  There is an important exception,
- * however: GROUP BY and ORDER BY expressions will have been pushed into the
- * subplan tlist unflattened.  If these values are also needed in the output
- * then we want to reference the subplan tlist element rather than recomputing
- * the expression.
+ * generated by flattening these selfsame expressions, so it should have all
+ * the required variables).  There is an important exception, however:
+ * depending on where we are in the plan tree, sort/group columns may have
+ * been pushed into the subplan tlist unflattened.  If these values are also
+ * needed in the output then we want to reference the subplan tlist element
+ * rather than recomputing the expression.
  */
 static void
 set_upper_references(PlannerInfo *root, Plan *plan, int rtoffset)
@@ -2129,7 +2129,8 @@ fix_join_expr_mutator(Node *node, fix_join_expr_context *context)
  *
  * An error is raised if no matching var can be found in the subplan tlist
  * --- so this routine should only be applied to nodes whose subplans'
- * targetlists were generated via flatten_tlist() or some such method.
+ * targetlists were generated by flattening the expressions used in the
+ * parent node.
  *
  * If itlist->has_non_vars is true, then we try to match whole subexpressions
  * against elements of the subplan tlist, so that we can avoid recomputing
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
@@ -31,6 +31,7 @@
 #include "optimizer/prep.h"
 #include "optimizer/subselect.h"
 #include "optimizer/tlist.h"
+#include "optimizer/var.h"
 #include "parser/parse_relation.h"
 #include "parser/parsetree.h"
 #include "rewrite/rewriteManip.h"
diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c
@@ -44,6 +44,7 @@
 #include "nodes/makefuncs.h"
 #include "optimizer/prep.h"
 #include "optimizer/tlist.h"
+#include "optimizer/var.h"
 #include "parser/parsetree.h"
 #include "parser/parse_coerce.h"
 #include "utils/rel.h"
@@ -167,7 +168,7 @@ preprocess_targetlist(PlannerInfo *root, List *tlist)
 		ListCell   *l;
 
 		vars = pull_var_clause((Node *) parse->returningList,
-							   PVC_RECURSE_AGGREGATES,
+							   PVC_RECURSE_AGGREGATES |
 							   PVC_INCLUDE_PLACEHOLDERS);
 		foreach(l, vars)
 		{
diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c
@@ -220,7 +220,7 @@ find_placeholders_in_expr(PlannerInfo *root, Node *expr)
 	 * convenient to use.
 	 */
 	vars = pull_var_clause(expr,
-						   PVC_RECURSE_AGGREGATES,
+						   PVC_RECURSE_AGGREGATES |
 						   PVC_INCLUDE_PLACEHOLDERS);
 	foreach(vl, vars)
 	{
@@ -354,7 +354,7 @@ fix_placeholder_input_needed_levels(PlannerInfo *root)
 	{
 		PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(lc);
 		List	   *vars = pull_var_clause((Node *) phinfo->ph_var->phexpr,
-										   PVC_RECURSE_AGGREGATES,
+										   PVC_RECURSE_AGGREGATES |
 										   PVC_INCLUDE_PLACEHOLDERS);
 
 		add_vars_to_targetlist(root, vars, phinfo->ph_eval_at, false);
diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c
@@ -100,34 +100,6 @@ tlist_member_match_var(Var *var, List *targetlist)
 	return NULL;
 }
 
-/*
- * flatten_tlist
- *	  Create a target list that only contains unique variables.
- *
- * Aggrefs and PlaceHolderVars in the input are treated according to
- * aggbehavior and phbehavior, for which see pull_var_clause().
- *
- * 'tlist' is the current target list
- *
- * Returns the "flattened" new target list.
- *
- * The result is entirely new structure sharing no nodes with the original.
- * Copying the Var nodes is probably overkill, but be safe for now.
- */
-List *
-flatten_tlist(List *tlist, PVCAggregateBehavior aggbehavior,
-			  PVCPlaceHolderBehavior phbehavior)
-{
-	List	   *vlist = pull_var_clause((Node *) tlist,
-										aggbehavior,
-										phbehavior);
-	List	   *new_tlist;
-
-	new_tlist = add_to_flat_tlist(NIL, vlist);
-	list_free(vlist);
-	return new_tlist;
-}
-
 /*
  * add_to_flat_tlist
  *		Add more items to a flattened tlist (if they're not already in it)
diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c
@@ -55,8 +55,7 @@ typedef struct
 typedef struct
 {
 	List	   *varlist;
-	PVCAggregateBehavior aggbehavior;
-	PVCPlaceHolderBehavior phbehavior;
+	int			flags;
 } pull_var_clause_context;
 
 typedef struct
@@ -497,17 +496,22 @@ locate_var_of_level_walker(Node *node,
  * pull_var_clause
  *	  Recursively pulls all Var nodes from an expression clause.
  *
- *	  Aggrefs are handled according to 'aggbehavior':
- *		PVC_REJECT_AGGREGATES		throw error if Aggref found
+ *	  Aggrefs are handled according to these bits in 'flags':
  *		PVC_INCLUDE_AGGREGATES		include Aggrefs in output list
  *		PVC_RECURSE_AGGREGATES		recurse into Aggref arguments
- *	  Vars within an Aggref's expression are included only in the last case.
+ *		neither flag				throw error if Aggref found
+ *	  Vars within an Aggref's expression are included in the result only
+ *	  when PVC_RECURSE_AGGREGATES is specified.
  *
- *	  PlaceHolderVars are handled according to 'phbehavior':
- *		PVC_REJECT_PLACEHOLDERS		throw error if PlaceHolderVar found
+ *	  PlaceHolderVars are handled according to these bits in 'flags':
  *		PVC_INCLUDE_PLACEHOLDERS	include PlaceHolderVars in output list
  *		PVC_RECURSE_PLACEHOLDERS	recurse into PlaceHolderVar arguments
- *	  Vars within a PHV's expression are included only in the last case.
+ *		neither flag				throw error if PlaceHolderVar found
+ *	  Vars within a PHV's expression are included in the result only
+ *	  when PVC_RECURSE_PLACEHOLDERS is specified.
+ *
+ *	  GroupingFuncs are treated mostly like Aggrefs, and so do not need
+ *	  their own flag bits.
  *
  *	  CurrentOfExpr nodes are ignored in all cases.
  *
@@ -521,14 +525,18 @@ locate_var_of_level_walker(Node *node,
  * of sublinks to subplans!
  */
 List *
-pull_var_clause(Node *node, PVCAggregateBehavior aggbehavior,
-				PVCPlaceHolderBehavior phbehavior)
+pull_var_clause(Node *node, int flags)
 {
 	pull_var_clause_context context;
 
+	/* Assert that caller has not specified inconsistent flags */
+	Assert((flags & (PVC_INCLUDE_AGGREGATES | PVC_RECURSE_AGGREGATES))
+		   != (PVC_INCLUDE_AGGREGATES | PVC_RECURSE_AGGREGATES));
+	Assert((flags & (PVC_INCLUDE_PLACEHOLDERS | PVC_RECURSE_PLACEHOLDERS))
+		   != (PVC_INCLUDE_PLACEHOLDERS | PVC_RECURSE_PLACEHOLDERS));
+
 	context.varlist = NIL;
-	context.aggbehavior = aggbehavior;
-	context.phbehavior = phbehavior;
+	context.flags = flags;
 
 	pull_var_clause_walker(node, &context);
 	return context.varlist;
@@ -550,62 +558,58 @@ pull_var_clause_walker(Node *node, pull_var_clause_context *context)
 	{
 		if (((Aggref *) node)->agglevelsup != 0)
 			elog(ERROR, "Upper-level Aggref found where not expected");
-		switch (context->aggbehavior)
+		if (context->flags & PVC_INCLUDE_AGGREGATES)
+		{
+			context->varlist = lappend(context->varlist, node);
+			/* we do NOT descend into the contained expression */
+			return false;
+		}
+		else if (context->flags & PVC_RECURSE_AGGREGATES)
 		{
-			case PVC_REJECT_AGGREGATES:
-				elog(ERROR, "Aggref found where not expected");
-				break;
-			case PVC_INCLUDE_AGGREGATES:
-				context->varlist = lappend(context->varlist, node);
-				/* we do NOT descend into the contained expression */
-				return false;
-			case PVC_RECURSE_AGGREGATES:
-				/* ignore the aggregate, look at its argument instead */
-				break;
+			/* fall through to recurse into the aggregate's arguments */
 		}
+		else
+			elog(ERROR, "Aggref found where not expected");
 	}
 	else if (IsA(node, GroupingFunc))
 	{
 		if (((GroupingFunc *) node)->agglevelsup != 0)
 			elog(ERROR, "Upper-level GROUPING found where not expected");
-		switch (context->aggbehavior)
+		if (context->flags & PVC_INCLUDE_AGGREGATES)
 		{
-			case PVC_REJECT_AGGREGATES:
-				elog(ERROR, "GROUPING found where not expected");
-				break;
-			case PVC_INCLUDE_AGGREGATES:
-				context->varlist = lappend(context->varlist, node);
-				/* we do NOT descend into the contained expression */
-				return false;
-			case PVC_RECURSE_AGGREGATES:
-
-				/*
-				 * we do NOT descend into the contained expression, even if
-				 * the caller asked for it, because we never actually evaluate
-				 * it - the result is driven entirely off the associated GROUP
-				 * BY clause, so we never need to extract the actual Vars
-				 * here.
-				 */
-				return false;
+			context->varlist = lappend(context->varlist, node);
+			/* we do NOT descend into the contained expression */
+			return false;
+		}
+		else if (context->flags & PVC_RECURSE_AGGREGATES)
+		{
+			/*
+			 * We do NOT descend into the contained expression, even if the
+			 * caller asked for it, because we never actually evaluate it -
+			 * the result is driven entirely off the associated GROUP BY
+			 * clause, so we never need to extract the actual Vars here.
+			 */
+			return false;
 		}
+		else
+			elog(ERROR, "GROUPING found where not expected");
 	}
 	else if (IsA(node, PlaceHolderVar))
 	{
 		if (((PlaceHolderVar *) node)->phlevelsup != 0)
 			elog(ERROR, "Upper-level PlaceHolderVar found where not expected");
-		switch (context->phbehavior)
+		if (context->flags & PVC_INCLUDE_PLACEHOLDERS)
 		{
-			case PVC_REJECT_PLACEHOLDERS:
-				elog(ERROR, "PlaceHolderVar found where not expected");
-				break;
-			case PVC_INCLUDE_PLACEHOLDERS:
-				context->varlist = lappend(context->varlist, node);
-				/* we do NOT descend into the contained expression */
-				return false;
-			case PVC_RECURSE_PLACEHOLDERS:
-				/* ignore the placeholder, look at its argument instead */
-				break;
+			context->varlist = lappend(context->varlist, node);
+			/* we do NOT descend into the contained expression */
+			return false;
 		}
+		else if (context->flags & PVC_RECURSE_PLACEHOLDERS)
+		{
+			/* fall through to recurse into the placeholder's expression */
+		}
+		else
+			elog(ERROR, "PlaceHolderVar found where not expected");
 	}
 	return expression_tree_walker(node, pull_var_clause_walker,
 								  (void *) context);
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
diff --git a/src/include/optimizer/tlist.h b/src/include/optimizer/tlist.h
diff --git a/src/include/optimizer/var.h b/src/include/optimizer/var.h

Original file line number	Diff line number	Diff line change
`@@ -910,7 +910,7 @@ generate_base_implied_equalities_no_const(PlannerInfo *root,`
`910`	`910`	`{`
`911`	`911`	`EquivalenceMember cur_em = (EquivalenceMember ) lfirst(lc);`
`912`	`912`	`List vars = pull_var_clause((Node ) cur_em->em_expr,`
`913`		`- PVC_RECURSE_AGGREGATES,`
	`913`	`+ PVC_RECURSE_AGGREGATES \|`
`914`	`914`	`PVC_INCLUDE_PLACEHOLDERS);`
`915`	`915`
`916`	`916`	`add_vars_to_targetlist(root, vars, ec->ec_relids, false);`
Original file line number	Diff line number	Diff line change
`@@ -5335,7 +5335,7 @@ prepare_sort_from_pathkeys(Plan lefttree, List pathkeys,`
`5335`	`5335`
`5336`	`5336`	`sortexpr = em->em_expr;`
`5337`	`5337`	`exprvars = pull_var_clause((Node *) sortexpr,`
`5338`		`- PVC_INCLUDE_AGGREGATES,`
	`5338`	`+ PVC_INCLUDE_AGGREGATES \|`
`5339`	`5339`	`PVC_INCLUDE_PLACEHOLDERS);`
`5340`	`5340`	`foreach(k, exprvars)`
`5341`	`5341`	`{`