Avoid passing query tlist around separately from root->processed_tlist.

In the dim past, the planner kept the fully-processed version of the query
targetlist (the result of preprocess_targetlist) in grouping_planner's
local variable "tlist", and only grudgingly passed it to individual other
routines as needed.  Later we discovered a need to still have it available
after grouping_planner finishes, and invented the root->processed_tlist
field for that purpose, but it wasn't used internally to grouping_planner;
the tlist was still being passed around separately in the same places as
before.

Now comes a proposed patch to allow appendrel expansion to add entries
to the processed tlist, well after preprocess_targetlist has finished
its work.  To avoid having to pass around the tlist explicitly, it's
proposed to allow appendrel expansion to modify root->processed_tlist.
That makes aliasing the tlist with assorted parameters and local
variables really scary.  It would accidentally work as long as the
tlist is initially nonempty, because then the List header won't move
around, but it's not exactly hard to think of ways for that to break.
Aliased values are poor programming practice anyway.

Hence, get rid of local variables and parameters that can be identified
with root->processed_tlist, in favor of just using that field directly.
And adjust comments to match.  (Some of the new comments speak as though
it's already possible for appendrel expansion to modify the tlist; that's
not true yet, but will happen in a later patch.)

Discussion: https://postgr.es/m/9d7c5112-cb99-6a47-d3be-cf1ee6862a1d@lab.ntt.co.jp

Author: tglsfdc

src/backend/optimizer/plan/planagg.c

@@ -68,12 +68,9 @@ static Oid	fetch_agg_sort_op(Oid aggfnoid);
  * planner's state and invoking query_planner() on a modified version of
  * the query parsetree.  Thus, all preprocessing needed before query_planner()
  * must already be done.
- *
- * Note: we are passed the preprocessed targetlist separately, because it's
- * not necessarily equal to root->parse->targetList.
  */
 void
-preprocess_minmax_aggregates(PlannerInfo *root, List *tlist)
+preprocess_minmax_aggregates(PlannerInfo *root)
 {
 	Query	   *parse = root->parse;
 	FromExpr   *jtnode;
@@ -144,7 +141,7 @@ preprocess_minmax_aggregates(PlannerInfo *root, List *tlist)
 	 * all are MIN/MAX aggregates.  Stop as soon as we find one that isn't.
 	 */
 	aggs_list = NIL;
-	if (find_minmax_aggs_walker((Node *) tlist, &aggs_list))
+	if (find_minmax_aggs_walker((Node *) root->processed_tlist, &aggs_list))
 		return;
 	if (find_minmax_aggs_walker(parse->havingQual, &aggs_list))
 		return;
@@ -218,11 +215,14 @@ preprocess_minmax_aggregates(PlannerInfo *root, List *tlist)
 	 * consider_parallel value in it, but MinMaxAggPath paths are currently
 	 * never parallel-safe anyway, so that doesn't matter.  Likewise, it
 	 * doesn't matter that we haven't filled FDW-related fields in the rel.
+	 * Also, because there are no rowmarks, we know that the processed_tlist
+	 * doesn't need to change anymore, so making the pathtarget now is safe.
 	 */
 	grouped_rel = fetch_upper_rel(root, UPPERREL_GROUP_AGG, NULL);
 	add_path(grouped_rel, (Path *)
 			 create_minmaxagg_path(root, grouped_rel,
-								   create_pathtarget(root, tlist),
+								   create_pathtarget(root,
+													 root->processed_tlist),
 								   aggs_list,
 								   (List *) parse->havingQual));
 }
@@ -421,7 +421,7 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo,
 
 	/* Build suitable ORDER BY clause */
 	sortcl = makeNode(SortGroupClause);
-	sortcl->tleSortGroupRef = assignSortGroupRef(tle, tlist);
+	sortcl->tleSortGroupRef = assignSortGroupRef(tle, subroot->processed_tlist);
 	sortcl->eqop = eqop;
 	sortcl->sortop = sortop;
 	sortcl->nulls_first = nulls_first;
@@ -442,7 +442,7 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo,
 	subroot->tuple_fraction = 1.0;
 	subroot->limit_tuples = 1.0;
 
-	final_rel = query_planner(subroot, tlist, minmax_qp_callback, NULL);
+	final_rel = query_planner(subroot, minmax_qp_callback, NULL);
 
 	/*
 	 * Since we didn't go through subquery_planner() to handle the subquery,
@@ -476,7 +476,8 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo,
 	 * cheapest path.)
 	 */
 	sorted_path = apply_projection_to_path(subroot, final_rel, sorted_path,
-										   create_pathtarget(subroot, tlist));
+										   create_pathtarget(subroot,
+															 subroot->processed_tlist));
 
 	/*
 	 * Determine cost to get just the first row of the presorted path.

src/backend/optimizer/plan/planmain.c

@@ -42,8 +42,6 @@
  * (grouping_planner) can choose among the surviving paths for the rel.
  *
  * root describes the query to plan
- * tlist is the target list the query should produce
- *		(this is NOT necessarily root->parse->targetList!)
  * qp_callback is a function to compute query_pathkeys once it's safe to do so
  * qp_extra is optional extra data to pass to qp_callback
  *
@@ -54,7 +52,7 @@
  * (We cannot construct canonical pathkeys until that's done.)
  */
 RelOptInfo *
-query_planner(PlannerInfo *root, List *tlist,
+query_planner(PlannerInfo *root,
 			  query_pathkeys_callback qp_callback, void *qp_extra)
 {
 	Query	   *parse = root->parse;
@@ -179,7 +177,7 @@ query_planner(PlannerInfo *root, List *tlist,
 	 * restrictions.  Finally, we form a target joinlist for make_one_rel() to
 	 * work from.
 	 */
-	build_base_rel_tlists(root, tlist);
+	build_base_rel_tlists(root, root->processed_tlist);
 
 	find_placeholders_in_jointree(root);
 

src/backend/optimizer/plan/planner.c

@@ -95,7 +95,6 @@ create_upper_paths_hook_type create_upper_paths_hook = NULL;
 /* Passthrough data for standard_qp_callback */
 typedef struct
 {
-	List	   *tlist;			/* preprocessed query targetlist */
 	List	   *activeWindows;	/* active windows, if any */
 	List	   *groupClause;	/* overrides parse->groupClause */
 } standard_qp_extra;
@@ -182,15 +181,13 @@ static RelOptInfo *create_window_paths(PlannerInfo *root,
 					PathTarget *input_target,
 					PathTarget *output_target,
 					bool output_target_parallel_safe,
-					List *tlist,
 					WindowFuncLists *wflists,
 					List *activeWindows);
 static void create_one_window_path(PlannerInfo *root,
 					   RelOptInfo *window_rel,
 					   Path *path,
 					   PathTarget *input_target,
 					   PathTarget *output_target,
-					   List *tlist,
 					   WindowFuncLists *wflists,
 					   List *activeWindows);
 static RelOptInfo *create_distinct_paths(PlannerInfo *root,
@@ -1588,12 +1585,11 @@ inheritance_planner(PlannerInfo *root)
 		 * cleaner if we fixed nodeModifyTable.c to support zero child nodes,
 		 * but that probably wouldn't be a net win.)
 		 */
-		List	   *tlist;
 		Path	   *dummy_path;
 
 		/* tlist processing never got done, either */
-		tlist = root->processed_tlist = preprocess_targetlist(root);
-		final_rel->reltarget = create_pathtarget(root, tlist);
+		root->processed_tlist = preprocess_targetlist(root);
+		final_rel->reltarget = create_pathtarget(root, root->processed_tlist);
 
 		/* Make a dummy path, cf set_dummy_rel_pathlist() */
 		dummy_path = (Path *) create_append_path(NULL, final_rel, NIL, NIL,
@@ -1693,7 +1689,6 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 				 double tuple_fraction)
 {
 	Query	   *parse = root->parse;
-	List	   *tlist;
 	int64		offset_est = 0;
 	int64		count_est = 0;
 	double		limit_tuples = -1.0;
@@ -1746,20 +1741,17 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 
 		/*
 		 * We should not need to call preprocess_targetlist, since we must be
-		 * in a SELECT query node.  Instead, use the targetlist returned by
-		 * plan_set_operations (since this tells whether it returned any
+		 * in a SELECT query node.  Instead, use the processed_tlist returned
+		 * by plan_set_operations (since this tells whether it returned any
 		 * resjunk columns!), and transfer any sort key information from the
 		 * original tlist.
 		 */
 		Assert(parse->commandType == CMD_SELECT);
 
-		tlist = root->processed_tlist;	/* from plan_set_operations */
-
 		/* for safety, copy processed_tlist instead of modifying in-place */
-		tlist = postprocess_setop_tlist(copyObject(tlist), parse->targetList);
-
-		/* Save aside the final decorated tlist */
-		root->processed_tlist = tlist;
+		root->processed_tlist =
+			postprocess_setop_tlist(copyObject(root->processed_tlist),
+									parse->targetList);
 
 		/* Also extract the PathTarget form of the setop result tlist */
 		final_target = current_rel->cheapest_total_path->pathtarget;
@@ -1791,7 +1783,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 		Assert(parse->distinctClause == NIL);
 		root->sort_pathkeys = make_pathkeys_for_sortclauses(root,
 															parse->sortClause,
-															tlist);
+															root->processed_tlist);
 	}
 	else
 	{
@@ -1831,17 +1823,14 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 				parse->groupClause = preprocess_groupclause(root, NIL);
 		}
 
-		/* Preprocess targetlist */
-		tlist = preprocess_targetlist(root);
-
 		/*
-		 * We are now done hacking up the query's targetlist.  Most of the
-		 * remaining planning work will be done with the PathTarget
-		 * representation of tlists, but save aside the full representation so
+		 * Preprocess targetlist.  Note that much of the remaining planning
+		 * work will be done with the PathTarget representation of tlists, but
+		 * we must also maintain the full representation of the final tlist so
 		 * that we can transfer its decoration (resnames etc) to the topmost
-		 * tlist of the finished Plan.
+		 * tlist of the finished Plan.  This is kept in processed_tlist.
 		 */
-		root->processed_tlist = tlist;
+		root->processed_tlist = preprocess_targetlist(root);
 
 		/*
 		 * Collect statistics about aggregates for estimating costs, and mark
@@ -1859,8 +1848,8 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 		MemSet(&agg_costs, 0, sizeof(AggClauseCosts));
 		if (parse->hasAggs)
 		{
-			get_agg_clause_costs(root, (Node *) tlist, AGGSPLIT_SIMPLE,
-								 &agg_costs);
+			get_agg_clause_costs(root, (Node *) root->processed_tlist,
+								 AGGSPLIT_SIMPLE, &agg_costs);
 			get_agg_clause_costs(root, parse->havingQual, AGGSPLIT_SIMPLE,
 								 &agg_costs);
 		}
@@ -1873,7 +1862,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 		 */
 		if (parse->hasWindowFuncs)
 		{
-			wflists = find_window_functions((Node *) tlist,
+			wflists = find_window_functions((Node *) root->processed_tlist,
 											list_length(parse->windowClause));
 			if (wflists->numWindowFuncs > 0)
 				activeWindows = select_active_windows(root, wflists);
@@ -1888,7 +1877,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 		 * duplicated in planagg.c.
 		 */
 		if (parse->hasAggs)
-			preprocess_minmax_aggregates(root, tlist);
+			preprocess_minmax_aggregates(root);
 
 		/*
 		 * Figure out whether there's a hard limit on the number of rows that
@@ -1908,7 +1897,6 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 			root->limit_tuples = limit_tuples;
 
 		/* Set up data needed by standard_qp_callback */
-		qp_extra.tlist = tlist;
 		qp_extra.activeWindows = activeWindows;
 		qp_extra.groupClause = (gset_data
 								? (gset_data->rollups ? linitial_node(RollupData, gset_data->rollups)->groupClause : NIL)
@@ -1921,17 +1909,18 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 		 * We also generate (in standard_qp_callback) pathkey representations
 		 * of the query's sort clause, distinct clause, etc.
 		 */
-		current_rel = query_planner(root, tlist,
-									standard_qp_callback, &qp_extra);
+		current_rel = query_planner(root, standard_qp_callback, &qp_extra);
 
 		/*
 		 * Convert the query's result tlist into PathTarget format.
 		 *
-		 * Note: it's desirable to not do this till after query_planner(),
+		 * Note: this cannot be done before query_planner() has performed
+		 * appendrel expansion, because that might add resjunk entries to
+		 * root->processed_tlist.  Waiting till afterwards is also helpful
 		 * because the target width estimates can use per-Var width numbers
 		 * that were obtained within query_planner().
 		 */
-		final_target = create_pathtarget(root, tlist);
+		final_target = create_pathtarget(root, root->processed_tlist);
 		final_target_parallel_safe =
 			is_parallel_safe(root, (Node *) final_target->exprs);
 
@@ -2087,7 +2076,6 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 											  grouping_target,
 											  sort_input_target,
 											  sort_input_target_parallel_safe,
-											  tlist,
 											  wflists,
 											  activeWindows);
 			/* Fix things up if sort_input_target contains SRFs */
@@ -3455,7 +3443,7 @@ standard_qp_callback(PlannerInfo *root, void *extra)
 {
 	Query	   *parse = root->parse;
 	standard_qp_extra *qp_extra = (standard_qp_extra *) extra;
-	List	   *tlist = qp_extra->tlist;
+	List	   *tlist = root->processed_tlist;
 	List	   *activeWindows = qp_extra->activeWindows;
 
 	/*
@@ -4401,7 +4389,6 @@ consider_groupingsets_paths(PlannerInfo *root,
  * input_rel: contains the source-data Paths
  * input_target: result of make_window_input_target
  * output_target: what the topmost WindowAggPath should return
- * tlist: query's target list (needed to look up pathkeys)
  * wflists: result of find_window_functions
  * activeWindows: result of select_active_windows
  *
@@ -4413,7 +4400,6 @@ create_window_paths(PlannerInfo *root,
 					PathTarget *input_target,
 					PathTarget *output_target,
 					bool output_target_parallel_safe,
-					List *tlist,
 					WindowFuncLists *wflists,
 					List *activeWindows)
 {
@@ -4456,7 +4442,6 @@ create_window_paths(PlannerInfo *root,
 								   path,
 								   input_target,
 								   output_target,
-								   tlist,
 								   wflists,
 								   activeWindows);
 	}
@@ -4490,7 +4475,6 @@ create_window_paths(PlannerInfo *root,
  * path: input Path to use (must return input_target)
  * input_target: result of make_window_input_target
  * output_target: what the topmost WindowAggPath should return
- * tlist: query's target list (needed to look up pathkeys)
  * wflists: result of find_window_functions
  * activeWindows: result of select_active_windows
  */
@@ -4500,7 +4484,6 @@ create_one_window_path(PlannerInfo *root,
 					   Path *path,
 					   PathTarget *input_target,
 					   PathTarget *output_target,
-					   List *tlist,
 					   WindowFuncLists *wflists,
 					   List *activeWindows)
 {
@@ -4531,7 +4514,7 @@ create_one_window_path(PlannerInfo *root,
 
 		window_pathkeys = make_pathkeys_for_window(root,
 												   wc,
-												   tlist);
+												   root->processed_tlist);
 
 		/* Sort if necessary */
 		if (!pathkeys_contained_in(window_pathkeys, path->pathkeys))

src/include/nodes/pathnodes.h

@@ -307,8 +307,13 @@ struct PlannerInfo
 	struct PathTarget *upper_targets[UPPERREL_FINAL + 1];
 
 	/*
-	 * grouping_planner passes back its final processed targetlist here, for
-	 * use in relabeling the topmost tlist of the finished Plan.
+	 * The fully-processed targetlist is kept here.  It differs from
+	 * parse->targetList in that (for INSERT and UPDATE) it's been reordered
+	 * to match the target table, and defaults have been filled in.  Also,
+	 * additional resjunk targets may be present.  preprocess_targetlist()
+	 * does most of this work, but note that more resjunk targets can get
+	 * added during appendrel expansion.  (Hence, upper_targets mustn't get
+	 * set up till after that.)
 	 */
 	List	   *processed_tlist;
 

src/include/optimizer/planmain.h

@@ -27,13 +27,13 @@ typedef void (*query_pathkeys_callback) (PlannerInfo *root, void *extra);
 /*
  * prototypes for plan/planmain.c
  */
-extern RelOptInfo *query_planner(PlannerInfo *root, List *tlist,
+extern RelOptInfo *query_planner(PlannerInfo *root,
 			  query_pathkeys_callback qp_callback, void *qp_extra);
 
 /*
  * prototypes for plan/planagg.c
  */
-extern void preprocess_minmax_aggregates(PlannerInfo *root, List *tlist);
+extern void preprocess_minmax_aggregates(PlannerInfo *root);
 
 /*
  * prototypes for plan/createplan.c