From b6736b127a93b0203ad8a5ab9dd1514135606517 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 24 Sep 2025 17:30:22 -0400 Subject: [PATCH v10 2/2] Use a list of subplan names instead of a list of "allroots". I don't like constructing a list of all PlannerInfo roots: it's partially redundant with the subroots list, and it's far from clear how to handle transient roots such as the one manufactured by pull_up_simple_subquery. This patch proposes that we should instead just store a list of assigned subplan names. One advantage is that choose_plan_name can immediately enter the new name into the list, instead of relying on the caller to get it right later. Apropos to that point, I found that build_minmax_path was putting the wrong root into the list, so that it was perfectly capable of assigning "minmax_1" over and over, as indeed is visible in a couple of incorrect regression test changes in v9-0001. I remain unsure whether it's OK for pull_up_simple_subquery to just do + subroot->plan_name = root->plan_name; but at least now we won't have two roots with the same plan_name in the allroots list. There's no way that that's helpful. Also fix the missed use of choose_plan_name in SS_process_ctes: without that, we can't really promise that we'll attach unique names to CTE subplans. --- src/backend/optimizer/plan/planagg.c | 3 -- src/backend/optimizer/plan/planner.c | 35 ++++++++++++++--------- src/backend/optimizer/plan/subselect.c | 5 ++-- src/backend/optimizer/prep/prepjointree.c | 3 -- src/include/nodes/pathnodes.h | 6 ++-- src/test/regress/expected/aggregates.out | 6 ++-- src/test/regress/expected/inherit.out | 2 +- 7 files changed, 31 insertions(+), 29 deletions(-) diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c index 0ce35cabaf5..a2ac58d246e 100644 --- a/src/backend/optimizer/plan/planagg.c +++ b/src/backend/optimizer/plan/planagg.c @@ -362,9 +362,6 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo, /* and we haven't created PlaceHolderInfos, either */ Assert(subroot->placeholder_list == NIL); - /* Add this to list of all PlannerInfo objects. */ - root->glob->allroots = lappend(root->glob->allroots, root); - /*---------- * Generate modified query of the form * (SELECT col FROM tab diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index acd1356a721..3b130e724f7 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -631,6 +631,7 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions, * * glob is the global state for the current planner run. * parse is the querytree produced by the parser & rewriter. + * plan_name is the name to assign to this subplan (NULL at the top level). * parent_root is the immediate parent Query's info (NULL at the top level). * hasRecursion is true if this is a recursive WITH query. * tuple_fraction is the fraction of tuples we expect will be retrieved. @@ -712,9 +713,6 @@ subquery_planner(PlannerGlobal *glob, Query *parse, char *plan_name, root->non_recursive_path = NULL; root->partColsUpdated = false; - /* Add this to list of all PlannerInfo objects. */ - root->glob->allroots = lappend(root->glob->allroots, root); - /* * Create the top-level join domain. This won't have valid contents until * deconstruct_jointree fills it in, but the node needs to exist before @@ -8840,7 +8838,9 @@ create_partial_unique_paths(PlannerInfo *root, RelOptInfo *input_rel, } /* - * Choose a unique plan name for subroot. + * Choose a unique name for some subroot. + * + * Modifies glob->subplanNames to track names already used. */ char * choose_plan_name(PlannerGlobal *glob, const char *name, bool always_number) @@ -8848,18 +8848,17 @@ choose_plan_name(PlannerGlobal *glob, const char *name, bool always_number) unsigned n; /* - * If a numeric suffix is not required, then search the list of roots for - * a plan with the requested name. If none is found, then we can use the - * provided name without modification. + * If a numeric suffix is not required, then search the list of + * previously-assigned names for a match. If none is found, then we can + * use the provided name without modification. */ if (!always_number) { bool found = false; - foreach_node(PlannerInfo, root, glob->allroots) + foreach_ptr(char, subplan_name, glob->subplanNames) { - if (root->plan_name != NULL && - strcmp(name, root->plan_name) == 0) + if (strcmp(subplan_name, name) == 0) { found = true; break; @@ -8867,7 +8866,13 @@ choose_plan_name(PlannerGlobal *glob, const char *name, bool always_number) } if (!found) - return pstrdup(name); + { + /* pstrdup here is just to avoid cast-away-const */ + char *chosen_name = pstrdup(name); + + glob->subplanNames = lappend(glob->subplanNames, chosen_name); + return chosen_name; + } } /* @@ -8880,10 +8885,9 @@ choose_plan_name(PlannerGlobal *glob, const char *name, bool always_number) char *proposed_name = psprintf("%s_%u", name, n); bool found = false; - foreach_node(PlannerInfo, root, glob->allroots) + foreach_ptr(char, subplan_name, glob->subplanNames) { - if (root->plan_name != NULL && - strcmp(proposed_name, root->plan_name) == 0) + if (strcmp(subplan_name, proposed_name) == 0) { found = true; break; @@ -8891,7 +8895,10 @@ choose_plan_name(PlannerGlobal *glob, const char *name, bool always_number) } if (!found) + { + glob->subplanNames = lappend(glob->subplanNames, proposed_name); return proposed_name; + } pfree(proposed_name); } diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index 5f8306bc421..14192a13236 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -967,8 +967,9 @@ SS_process_ctes(PlannerInfo *root) * Generate Paths for the CTE query. Always plan for full retrieval * --- we don't have enough info to predict otherwise. */ - subroot = subquery_planner(root->glob, subquery, cte->ctename, root, - cte->cterecursive, 0.0, NULL); + subroot = subquery_planner(root->glob, subquery, + choose_plan_name(root->glob, cte->ctename, false), + root, cte->cterecursive, 0.0, NULL); /* * Since the current query level doesn't yet contain any RTEs, it diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index 2ec13637d16..563be151a4d 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -1388,9 +1388,6 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte, subroot->non_recursive_path = NULL; /* We don't currently need a top JoinDomain for the subroot */ - /* Add new subroot to master list of PlannerInfo objects. */ - root->glob->allroots = lappend(root->glob->allroots, subroot); - /* No CTEs to worry about */ Assert(subquery->cteList == NIL); diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index a341b01a1e1..7ee9a7a68d8 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -110,8 +110,8 @@ typedef struct PlannerGlobal /* PlannerInfos for SubPlan nodes */ List *subroots pg_node_attr(read_write_ignore); - /* every PlannerInfo regardless of whether it's an InitPlan/SubPlan */ - List *allroots pg_node_attr(read_write_ignore); + /* names already used for subplans (list of C strings) */ + List *subplanNames pg_node_attr(read_write_ignore); /* indices of subplans that require REWIND */ Bitmapset *rewindPlanIDs; @@ -231,7 +231,7 @@ struct PlannerInfo /* NULL at outermost Query */ PlannerInfo *parent_root pg_node_attr(read_write_ignore); - /* Name for EXPLAIN and debugging purposes */ + /* Subplan name for EXPLAIN and debugging purposes (NULL at top level) */ char *plan_name; /* diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out index a9503e810c5..1f84db2f361 100644 --- a/src/test/regress/expected/aggregates.out +++ b/src/test/regress/expected/aggregates.out @@ -1269,7 +1269,7 @@ explain (costs off) -> Index Only Scan Backward using minmaxtest2i on minmaxtest2 minmaxtest_3 Index Cond: (f1 IS NOT NULL) -> Index Only Scan using minmaxtest3i on minmaxtest3 minmaxtest_4 - InitPlan minmax_1 + InitPlan minmax_2 -> Limit -> Merge Append Sort Key: minmaxtest_5.f1 DESC @@ -1305,7 +1305,7 @@ explain (costs off) -> Index Only Scan Backward using minmaxtest2i on minmaxtest2 minmaxtest_3 Index Cond: (f1 IS NOT NULL) -> Index Only Scan using minmaxtest3i on minmaxtest3 minmaxtest_4 - InitPlan minmax_1 + InitPlan minmax_2 -> Limit -> Merge Append Sort Key: minmaxtest_5.f1 DESC @@ -1317,7 +1317,7 @@ explain (costs off) Index Cond: (f1 IS NOT NULL) -> Index Only Scan Backward using minmaxtest3i on minmaxtest3 minmaxtest_9 -> Sort - Sort Key: ((InitPlan minmax_1).col1), ((InitPlan minmax_1).col1) + Sort Key: ((InitPlan minmax_1).col1), ((InitPlan minmax_2).col1) -> Result Replaces: MinMaxAggregate (27 rows) diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index 6dbbd26f56b..0490a746555 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -3264,7 +3264,7 @@ explain (costs off) select min(a), max(a) from parted_minmax where b = '12345'; -> Limit -> Index Only Scan using parted_minmax1i on parted_minmax1 parted_minmax Index Cond: ((a IS NOT NULL) AND (b = '12345'::text)) - InitPlan minmax_1 + InitPlan minmax_2 -> Limit -> Index Only Scan Backward using parted_minmax1i on parted_minmax1 parted_minmax_1 Index Cond: ((a IS NOT NULL) AND (b = '12345'::text)) -- 2.43.7