10000 Avoid passing query tlist around separately from root->processed_tlist. · postgres/postgres@333ed24 · GitHub
[go: up one dir, main page]
Skip to content

Commit 333ed24

Browse files
tglsfdctglsfdc
committed
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
1 parent 9938d11 commit 333ed24

File tree

5 files changed

+45
-58
lines changed

5 files changed

+45
-58
lines changed

‎src/backend/optimizer/plan/planagg.c

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -68,12 +68,9 @@ static Oid fetch_agg_sort_op(Oid aggfnoid);
6868
* planner's state and invoking query_planner() on a modified version of
6969
* the query parsetree. Thus, all preprocessing needed before query_planner()
7070
* must already be done.
71-
*
72-
* Note: we are passed the preprocessed targetlist separately, because it's
73-
* not necessarily equal to root->parse->targetList.
7471
*/
7572
void
76-
preprocess_minmax_aggregates(PlannerInfo *root, List *tlist)
73+
preprocess_minmax_aggregates(PlannerInfo *root)
7774
{
7875
Query *parse = root->parse;
7976
FromExpr *jtnode;
@@ -144,7 +141,7 @@ preprocess_minmax_aggregates(PlannerInfo *root, List *tlist)
144141
* all are MIN/MAX aggregates. Stop as soon as we find one that isn't.
145142
*/
146143
aggs_list = NIL;
147-
if (find_minmax_aggs_walker((Node *) tlist, &aggs_list))
144+
if (find_minmax_aggs_walker((Node *) root->processed_tlist, &aggs_list))
148145
return;
149146
if (find_minmax_aggs_walker(parse->havingQual, &aggs_list))
150147
return;
@@ -218,11 +215,14 @@ preprocess_minmax_aggregates(PlannerInfo *root, List *tlist)
218215
* consider_parallel value in it, but MinMaxAggPath paths are currently
219216
* never parallel-safe anyway, so that doesn't matter. Likewise, it
220217
* doesn't matter that we haven't filled FDW-related fields in the rel.
218+
* Also, because there are no rowmarks, we know that the processed_tlist
219+
* doesn't need to change anymore, so making the pathtarget now is safe.
221220
*/
222221
grouped_rel = fetch_upper_rel(root, UPPERREL_GROUP_AGG, NULL);
223222
add_path(grouped_rel, (Path *)
224223
create_minmaxagg_path(root, grouped_rel,
225-
create_pathtarget(root, tlist),
224+
create_pathtarget(root,
225+
root->processed_tlist),
226226
aggs_list,
227227
(List *) parse->havingQual));
228228
}
@@ -421,7 +421,7 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo,
421421

422422
/* Build suitable ORDER BY clause */
423423
sortcl = makeNode(SortGroupClause);
424-
sortcl->tleSortGroupRef = assignSortGroupRef(tle, tlist);
424+
sortcl->tleSortGroupRef = assignSortGroupRef(tle, subroot->processed_tlist);
425425
sortcl->eqop = eqop;
426426
sortcl->sortop = sortop;
427427
sortcl->nulls_first = nulls_first;
@@ -442,7 +442,7 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo,
442442
subroot->tuple_fraction = 1.0;
443443
subroot->limit_tuples = 1.0;
444444

445-
final_rel = query_planner(subroot, tlist, minmax_qp_callback, NULL);
445+
final_rel = query_planner(subroot, minmax_qp_callback, NULL);
446446

447447
/*
448448
* Since we didn't go through subquery_planner() to handle the subquery,
@@ -476,7 +476,8 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo,
476476
* cheapest path.)
477477
*/
478478
sorted_path = apply_projection_to_path(subroot, final_rel, sorted_path,
479-
create_pathtarget(subroot, tlist));
479+
create_pathtarget(subroot,
480+
subroot->processed_tlist));
480481

481482
/*
482483
* Determine cost to get just the first row of the presorted path.

‎src/backend/optimizer/plan/planmain.c

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,6 @@
4242
* (grouping_planner) can choose among the surviving paths for the rel.
4343
*
4444
* root describes the query to plan
45-
* tlist is the target list the query should produce
46-
* (this is NOT necessarily root->parse->targetList!)
4745
* qp_callback is a function to compute query_pathkeys once it's safe to do so
4846
* qp_extra is optional extra data to pass to qp_callback
4947
*
@@ -54,7 +52,7 @@
5452
* (We cannot construct canonical pathkeys until that's done.)
5553
*/
5654
RelOptInfo *
57-
query_planner(PlannerInfo *root, List *tlist,
55+
query_planner(PlannerInfo *root,
5856
query_pathkeys_callback qp_callback, void *qp_extra)
5957
{
6058
Query *parse = root->parse;
@@ -179,7 +177,7 @@ query_planner(PlannerInfo *root, List *tlist,
179177
* restrictions. Finally, we form a target joinlist for make_one_rel() to
180178
* work from.
181179
*/
182-
build_base_rel_tlists(root, tlist);
180+
build_base_rel_tlists(root, root->processed_tlist);
183181

184182
find_placeholders_in_jointree(root);
185183

‎src/backend/optimizer/plan/planner.c

Lines changed: 24 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,6 @@ create_upper_paths_hook_type create_upper_paths_hook = NULL;
9595
/* Passthrough data for standard_qp_callback */
9696
typedef struct
9797
{
98-
List *tlist; /* preprocessed query targetlist */
9998
List *activeWindows; /* active windows, if any */
10099
List *groupClause; /* overrides parse->groupClause */
101100
} standard_ 10000 qp_extra;
@@ -182,15 +181,13 @@ static RelOptInfo *create_window_paths(PlannerInfo *root,
182181
PathTarget *input_target,
183182
PathTarget *output_target,
184183
bool output_target_parallel_safe,
185-
List *tlist,
186184
WindowFuncLists *wflists,
187185
List *activeWindows);
188186
static void create_one_window_path(PlannerInfo *root,
189187
RelOptInfo *window_rel,
190188
Path *path,
191189
PathTarget *input_target,
192190
PathTarget *output_target,
193-
List *tlist,
194191
WindowFuncLists *wflists,
195192
List *activeWindows);
196193
static RelOptInfo *create_distinct_paths(PlannerInfo *root,
@@ -1588,12 +1585,11 @@ inheritance_planner(PlannerInfo *root)
15881585
* cleaner if we fixed nodeModifyTable.c to support zero child nodes,
15891586
* but that probably wouldn't be a net win.)
15901587
*/
1591-
List *tlist;
15921588
Path *dummy_path;
15931589

15941590
/* tlist processing never got done, either */
1595-
tlist = root->processed_tlist = preprocess_targetlist(root);
1596-
final_rel->reltarget = create_pathtarget(root, tlist);
1591+
root->processed_tlist = preprocess_targetlist(root);
1592+
final_rel->reltarget = create_pathtarget(root, root->processed_tlist);
15971593

15981594
/* Make a dummy path, cf set_dummy_rel_pathlist() */
15991595
dummy_path = (Path *) create_append_path(NULL, final_rel, NIL, NIL,
@@ -1693,7 +1689,6 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
16931689
double tuple_fraction)
16941690
{
16951691
Query *parse = root->parse;
1696-
List *tlist;
16971692
int64 offset_est = 0;
16981693
int64 count_est = 0;
16991694
double limit_tuples = -1.0;
@@ -1746,20 +1741,17 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
17461741

17471742
/*
17481743
* We should not need to call preprocess_targetlist, since we must be
1749-
* in a SELECT query node. Instead, use the targetlist returned by
1750-
* plan_set_operations (since this tells whether it returned any
1744+
* in a SELECT query node. Instead, use the processed_tlist returned
1745+
* by plan_set_operations (since this tells whether it returned any
17511746
* resjunk columns!), and transfer any sort key information from the
17521747
* original tlist.
17531748
*/
17541749
Assert(parse->commandType == CMD_SELECT);
17551750

1756-
tlist = root->processed_tlist; /* from plan_set_operations */
1757-
17581751
/* for safety, copy processed_tlist instead of modifying in-place */
1759-
tlist = postprocess_setop_tlist(copyObject(tlist), parse->targetList);
1760-
1761-
/* Save aside the final decorated tlist */
1762-
root->processed_tlist = tlist;
1752+
root->processed_tlist =
1753+
postprocess_setop_tlist(copyObject(root->processed_tlist),
1754+
parse->targetList);
17631755

17641756
/* Also extract the PathTarget form of the setop result tlist */
17651757
final_target = current_rel->cheapest_total_path->pathtarget;
@@ -1791,7 +1783,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
17911783
Assert(parse->distinctClause == NIL);
17921784
root->sort_pathkeys = make_pathkeys_for_sortclauses(root,
17931785
parse->sortClause,
1794-
tlist);
1786+
root->processed_tlist);
17951787
}
17961788
else
17971789
{
@@ -1831,17 +1823,14 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
18311823
parse->groupClause = preprocess_groupclause(root, NIL);
18321824
}
18331825

1834-
/* Preprocess targetlist */
1835-
tlist = preprocess_targetlist(root);
1836-
18371826
/*
1838-
* We are now done hacking up the query's targetlist. Most of the
1839-
* remaining planning work will be done with the PathTarget
1840-
* representation of tlists, but save aside the full representation so
1827+
* Preprocess targetlist. Note that much of the remaining planning
1828+
* work will be done with the PathTarget representation of tlists, but
1829+
* we must also maintain the full representation of the final tlist so
18411830
* that we can transfer its decoration (resnames etc) to the topmost
1842-
* tlist of the finished Plan.
1831+
* tlist of the finished Plan. This is kept in processed_tlist.
18431832
*/
1844-
root->processed_tlist = tlist;
1833+
root->processed_tlist = preprocess_targetlist(root);
18451834

18461835
/*
18471836
* Collect statistics about aggregates for estimating costs, and mark
@@ -1859,8 +1848,8 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
18591848
MemSet(&agg_costs, 0, sizeof(AggClauseCosts));
18601849
if (parse->hasAggs)
18611850
{
1862-
get_agg_clause_costs(root, (Node *) tlist, AGGSPLIT_SIMPLE,
1863-
&agg_costs);
1851+
get_agg_clause_costs(root, (Node *) root->processed_tlist,
1852+
AGGSPLIT_SIMPLE, &agg_costs);
18641853
get_agg_clause_costs(root, parse->havingQual, AGGSPLIT_SIMPLE,
18651854
&agg_costs);
18661855
}
@@ -1873,7 +1862,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
18731862
*/
18741863
if (parse->hasWindowFuncs)
18751864
{
1876-
wflists = find_window_functions((Node *) tlist,
1865+
wflists = find_window_functions((Node *) root->processed_tlist,
18771866
list_length(parse->windowClause));
18781867
if (wflists->numWindowFuncs > 0)
18791868
activeWindows = select_active_windows(root, wflists);
@@ -1888,7 +1877,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
18881877
* duplicated in planagg.c.
18891878
*/
18901879
if (parse->hasAggs)
1891-
preprocess_minmax_aggregates(root, tlist);
1880+
preprocess_minmax_aggregates(root);
18921881

18931882
/*
18941883
* 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,
19081897
root->limit_tuples = limit_tuples;
19091898

19101899
/* Set up data needed by standard_qp_callback */
1911-
qp_extra.tlist = tlist;
19121900
qp_extra.activeWindows = activeWindows;
19131901
qp_extra.groupClause = (gset_data
19141902
? (gset_data->rollups ? linitial_node(RollupData, gset_data->rollups)->groupClause : NIL)
@@ -1921,17 +1909,18 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
19211909
* We also generate (in standard_qp_callback) pathkey representations
19221910
* of the query's sort clause, distinct clause, etc.
19231911
*/
1924-
current_rel = query_planner(root, tlist,
1925-
standard_qp_callback, &qp_extra);
1912+
current_rel = query_planner(root, standard_qp_callback, &qp_extra);
19261913

19271914
/*
19281915
* Convert the query's result tlist into PathTarget format.
19291916
*
1930-
* Note: it's desirable to not do this till after query_planner(),
1917+
* Note: this cannot be done before query_planner() has performed
1918+
* appendrel expansion, because that might add resjunk entries to
1919+
* root->processed_tlist. Waiting till afterwards is also helpful
19311920
* because the target width estimates can use per-Var width numbers
19321921
* that were obtained within query_planner().
19331922
*/
1934-
final_target = create_pathtarget(root, tlist);
1923+
final_target = create_pathtarget(root, root->processed_tlist);
19351924
final_target_parallel_safe =
19361925
is_parallel_safe(root, (Node *) final_target->exprs);
19371926

@@ -2087,7 +2076,6 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
20872076
grouping_target,
20882077
sort_input_target,
20892078
sort_input_target_parallel_safe,
2090-
tlist,
20912079
wflists,
20922080
activeWindows);
20932081
/* Fix things up if sort_input_target contains SRFs */
@@ -3455,7 +3443,7 @@ standard_qp_callback(PlannerInfo *root, void *extra)
34553443
{
34563444
Query *parse = root->parse;
34573445
standard_qp_extra *qp_extra = (standard_qp_extra *) extra;
3458-
List *tlist = qp_extra->tlist;
3446+
List *tlist = root->processed_tlist;
34593447
List *activeWindows = qp_extra->activeWindows;
34603448

34613449
/*
@@ -4401,7 +4389,6 @@ consider_groupingsets_paths(PlannerInfo *root,
44014389
* input_rel: contains the source-data Paths
44024390
* input_target: result of make_window_input_target
44034391
* output_target: what the topmost WindowAggPath should return
4404-
* tlist: query's target list (needed to look up pathkeys)
44054392
* wflists: result of find_window_functions
44064393
* activeWindows: result of select_active_windows
44074394
*
@@ -4413,7 +4400,6 @@ create_window_paths(PlannerInfo *root,
44134400
PathTarget *input_target,
44144401
PathTarget *output_target,
44154402
bool output_target_parallel_safe,
4416-
List *tlist,
44174403
WindowFuncLists *wflists,
44184404
List *activeWindows)
44194405
{
@@ -4456,7 +4442,6 @@ create_window_paths(PlannerInfo *root,
44564442
path,
44574443
input_target,
44584444
output_target,
4459-
tlist,
44604445
wflists,
44614446
activeWindows);
44624447
}
@@ -4490,7 +4475,6 @@ create_window_paths(PlannerInfo *root,
44904475
* path: input Path to use (must return input_target)
44914476
* input_target: result of make_window_input_target
44924477
* output_target: what the topmost WindowAggPath should return
4493-
* tlist: query's target list (needed to look up pathkeys)
44944478
* wflists: result of find_window_functions
44954479
* activeWindows: result of select_active_windows
44964480
*/
@@ -4500,7 +4484,6 @@ create_one_window_path(PlannerInfo *root,
45004484
Path *path,
45014485
PathTarget *input_target,
45024486
PathTarget *output_target,
4503-
List *tlist,
45044487
WindowFuncLists *wflists,
45054488
List *activeWindows)
45064489
{
@@ -4531,7 +4514,7 @@ create_one_window_path(PlannerInfo *root,
45314514

45324515
window_pathkeys = make_pathkeys_for_window(root,
45334516
wc,
4534-
tlist);
4517+
root->processed_tlist);
45354518

45364519
/* Sort if necessary */
45374520
if (!pathkeys_contained_in(window_pathkeys, path->pathkeys))

‎src/include/nodes/pathnodes.h

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -307,8 +307,13 @@ struct PlannerInfo
307307
struct PathTarget *upper_targets[UPPERREL_FINAL + 1];
308308

309309
/*
310-
* grouping_planner passes back its final processed targetlist here, for
311-
* use in relabeling the topmost tlist of the finished Plan.
310+
* The fully-processed targetlist is kept here. It differs from
311+
* parse->targetList in that (for INSERT and UPDATE) it's been reordered
312+
* to match the target table, and defaults have been filled in. Also,
313+
* additional resjunk targets may be present. preprocess_targetlist()
314+
* does most of this work, but note that more resjunk targets can get
315+
* added during appendrel expansion. (Hence, upper_targets mustn't get
316+
* set up till after that.)
312317
*/
313318
List *processed_tlist;
314319

‎src/include/optimizer/planmain.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,13 @@ typedef void (*query_pathkeys_callback) (PlannerInfo *root, void *extra);
2727
/*
2828
* prototypes for plan/planmain.c
2929
*/
30-
extern RelOptInfo *query_planner(PlannerInfo *root, List *tlist,
30+
extern RelOptInfo *query_planner(PlannerInfo *root,
3131
query_pathkeys_callback qp_callback, void *qp_extra);
3232

3333
/*
3434
* prototypes for plan/planagg.c
3535
*/
36-
extern void preprocess_minmax_aggregates(PlannerInfo *root, List *tlist);
36+
extern void preprocess_minmax_aggregates(PlannerInfo *root);
3737

3838
/*
3939
* prototypes for plan/createplan.c

0 commit comments

Comments
 (0)
0