Hi Ayoub, Andres, Tom, Thanks, all three of you. v2 is attached: no hash table, no extra memory, and the names stay as they are. A 4000-arm UNION plans in 21 ms (v18 20 ms, master 17.6 s), and every form stays within 0.92-1.18x of v18 up to 8000 arms.
On Wed, Oct 28, 2026 at 5:29 PM Ayoub Kazar <[email protected]> wrote: > And in non-assert builds, dynahash silently truncates the key at 74 > bytes, i think that risks collisions ? Worse than collisions, thanks for testing v1: with an 80-byte alias set by C code, two subqueries were enough for a non-assert build to loop forever in choose_plan_name(), ignoring cancel and terminate. v2 has no keys to cut and plans such aliases fine. On Wed, Oct 28, 2026 at 12:42 PM Andres Freund <[email protected]> wrote: > Are there halfway realistic cases where this is a problem? Only generated SQL. A UNION ALL of N aggregates, N correlated scalar subqueries or N ORed IN (SELECT ...) plan 2x slower than v18 from N = 100-200, and take over 100 ms from N = 500-1000. On Wed, Oct 28, 2026 at 1:31 PM Tom Lane <[email protected]> wrote: > Or just do it that way always. I think the requirement here is only > uniqueness and reproducibility. I tried that first. It works, but it renumbers names wherever base names mix, and users write those names into pg_plan_advice strings: its own alternatives test stops matching, since exists_to_any_1 becomes exists_to_any_2, and pg_plan_advice is in 19. So v2 keeps a counter per base name. Names are never released, so every smaller suffix is taken and the counter gives today's name. A suffixed name determines its base and suffix, split at the last underscore, so suffixed names can't collide with each other; only names used as given (CTE names, subquery aliases) can, and those are checked. No existing expected output changes, and a test-only build running the old algorithm alongside found no mismatch. check-world passes with cassert; REL_19_STABLE takes the patch as is. Regards, Manu
From 24778e0807441ee9a462cc619d496747301659cb Mon Sep 17 00:00:00 2001 From: Manu <[email protected]> Date: Wed, 28 Oct 2026 17:01:32 -0300 Subject: [PATCH v2] Avoid O(N^3) planning time in choose_plan_name() Since commit 8c49a484e8e, every subquery is given a unique name before it is planned. choose_plan_name() looked for the smallest free numeric suffix by trying 1, 2, 3, ... and scanning the list of all names assigned so far for each candidate, so planning a statement with N subplans that share a base name took O(N^3) time. A 4000-arm UNION took about 17 s to plan, against about 20 ms on v18. Names are never released, so for each base name remember the suffix after the last one used, and start the search there: every smaller suffix is taken, so the result is the same name as before. A suffixed name determines its base name and suffix, which are what come before and after its last underscore, so suffixed names cannot collide with each other and need not be stored at all; only the names used as given, such as CTE names and subquery aliases, can collide with them, and those are still checked. Each call now takes time proportional to the number of distinct base names and of names used as given, both small in the statements that were slow, and no more memory than before. The chosen names are unchanged, which matters because pg_plan_advice users write them into advice strings. Add a regression test for collisions with names used as given. Discussion: https://postgr.es/m/[email protected] --- src/backend/optimizer/plan/planner.c | 135 ++++++++++++++++-------- src/include/nodes/pathnodes.h | 5 +- src/test/regress/expected/subselect.out | 34 ++++++ src/test/regress/sql/subselect.sql | 12 +++ src/tools/pgindent/typedefs.list | 1 + 5 files changed, 143 insertions(+), 44 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index edaa42d..63ea391 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -9249,66 +9249,115 @@ create_partial_unique_paths(PlannerInfo *root, RelOptInfo *input_rel, } } +/* + * For a base name that has been given numeric suffixes, the next suffix to + * try; see choose_plan_name(). + */ +typedef struct SubplanNameSuffix +{ + const char *base; /* the base name */ + unsigned next_suffix; /* every smaller suffix is taken */ +} SubplanNameSuffix; + +/* + * Find the SubplanNameSuffix for a base name, creating it if asked to. + */ +static SubplanNameSuffix * +subplan_name_suffix(PlannerGlobal *glob, const char *base, bool create) +{ + SubplanNameSuffix *entry; + + foreach_ptr(SubplanNameSuffix, s, glob->subplanNameSuffixes) + { + if (strcmp(s->base, base) == 0) + return s; + } + if (!create) + return NULL; + + entry = palloc_object(SubplanNameSuffix); + entry->base = pstrdup(base); + entry->next_suffix = 1; + glob->subplanNameSuffixes = lappend(glob->subplanNameSuffixes, entry); + return entry; +} + +/* + * Is "name" already in use as a subplan name? See choose_plan_name(). + */ +static bool +subplan_name_in_use(PlannerGlobal *glob, const char *name) +{ + const char *suffix = strrchr(name, '_'); + + foreach_ptr(char, subplan_name, glob->subplanNames) + { + if (strcmp(subplan_name, name) == 0) + return true; + } + + /* + * A name given a numeric suffix is the base name, "_" and the suffix + * written without leading zeros, and it is in use if the suffix is below + * the base name's next_suffix. + */ + if (suffix != NULL && suffix[1] >= '1' && suffix[1] <= '9' && + strspn(suffix + 1, "0123456789") == strlen(suffix + 1) && + strlen(suffix + 1) <= 10) + { + char *base = pnstrdup(name, suffix - name); + SubplanNameSuffix *entry = subplan_name_suffix(glob, base, false); + + pfree(base); + if (entry != NULL && strtoul(suffix + 1, NULL, 10) < entry->next_suffix) + return true; + } + + return false; +} + /* * Choose a unique name for some subroot. * - * Modifies glob->subplanNames to track names already used. + * If always_number is false and the name is not in use, it is used as is and + * remembered in glob->subplanNames. Otherwise it gets the smallest positive + * numeric suffix that makes it unused. Names are never released, so for each + * base name we remember in glob->subplanNameSuffixes the suffix after the + * last one used, and every smaller one is known to be taken. Starting the + * search there gives the same name as starting at 1, but takes constant time + * instead of time proportional to the number of names; searching from 1 and + * scanning every name for each candidate made planning O(N^3) in the number + * of subplans. + * + * A name with a suffix determines its base name and suffix: they are what + * comes before and after the last underscore. So names given suffixes never + * collide with each other, and the names given suffixes need not be stored. + * Only the names used as is, such as CTE names and subquery aliases, can + * collide with them, and those are checked. */ char * choose_plan_name(PlannerGlobal *glob, const char *name, bool always_number) { + SubplanNameSuffix *entry; unsigned n; - /* - * 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) + if (!always_number && !subplan_name_in_use(glob, name)) { - bool found = false; - - foreach_ptr(char, subplan_name, glob->subplanNames) - { - if (strcmp(subplan_name, name) == 0) - { - found = true; - break; - } - } + /* pstrdup here is just to avoid cast-away-const */ + char *chosen_name = pstrdup(name); - if (!found) - { - /* pstrdup here is just to avoid cast-away-const */ - char *chosen_name = pstrdup(name); - - glob->subplanNames = lappend(glob->subplanNames, chosen_name); - return chosen_name; - } + glob->subplanNames = lappend(glob->subplanNames, chosen_name); + return chosen_name; } - /* - * If a numeric suffix is required or if the un-suffixed name is already - * in use, then loop until we find a positive integer that produces a - * novel name. - */ - for (n = 1; true; ++n) + entry = subplan_name_suffix(glob, name, true); + for (n = entry->next_suffix; true; ++n) { char *proposed_name = psprintf("%s_%u", name, n); - bool found = false; - foreach_ptr(char, subplan_name, glob->subplanNames) - { - if (strcmp(subplan_name, proposed_name) == 0) - { - found = true; - break; - } - } - - if (!found) + if (!subplan_name_in_use(glob, proposed_name)) { - glob->subplanNames = lappend(glob->subplanNames, proposed_name); + entry->next_suffix = n + 1; return proposed_name; } diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 1140c27..fe7f1eb 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -183,9 +183,12 @@ typedef struct PlannerGlobal /* PlannerInfos for SubPlan nodes */ List *subroots pg_node_attr(read_write_ignore); - /* names already used for subplans (list of C strings) */ + /* subplan names used as given (list of C strings) */ List *subplanNames pg_node_attr(read_write_ignore); + /* next numeric suffix to try for each base name; see choose_plan_name() */ + List *subplanNameSuffixes pg_node_attr(read_write_ignore); + /* indices of subplans that require REWIND */ Bitmapset *rewindPlanIDs; diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out index 6379bd6..ff7efb6 100644 --- a/src/test/regress/expected/subselect.out +++ b/src/test/regress/expected/subselect.out @@ -4075,3 +4075,37 @@ WHERE COALESCE(t1.id, -1) NOT IN (0 rows) ROLLBACK; +-- +-- Subplan names given a numeric suffix must not collide with names that are +-- used as given, such as CTE names, whichever is chosen first +-- +EXPLAIN (COSTS OFF) +WITH expr_1 AS MATERIALIZED (SELECT 1 AS x) +SELECT (SELECT x FROM expr_1), (SELECT 2 WHERE random() >= 0); + QUERY PLAN +---------------------------------------------------------------- + Result + CTE expr_1 + -> Result + InitPlan expr_2 + -> CTE Scan on expr_1 + InitPlan expr_3 + -> Result + One-Time Filter: (random() >= '0'::double precision) +(8 rows) + +EXPLAIN (COSTS OFF) +SELECT (SELECT 1 WHERE random() >= 0), + (WITH expr_1 AS MATERIALIZED (SELECT 2 AS x) SELECT x FROM expr_1); + QUERY PLAN +---------------------------------------------------------------- + Result + InitPlan expr_1 + -> Result + One-Time Filter: (random() >= '0'::double precision) + InitPlan expr_2 + -> CTE Scan on expr_1 + CTE expr_1_1 + -> Result +(8 rows) + diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql index c6cd275..a53c577 100644 --- a/src/test/regress/sql/subselect.sql +++ b/src/test/regress/sql/subselect.sql @@ -1790,3 +1790,15 @@ WHERE COALESCE(t1.id, -1) NOT IN (SELECT t1.val FROM not_null_tab t2 WHERE t2.val IS NOT NULL); ROLLBACK; + +-- +-- Subplan names given a numeric suffix must not collide with names that are +-- used as given, such as CTE names, whichever is chosen first +-- +EXPLAIN (COSTS OFF) +WITH expr_1 AS MATERIALIZED (SELECT 1 AS x) +SELECT (SELECT x FROM expr_1), (SELECT 2 WHERE random() >= 0); + +EXPLAIN (COSTS OFF) +SELECT (SELECT 1 WHERE random() >= 0), + (WITH expr_1 AS MATERIALIZED (SELECT 2 AS x) SELECT x FROM expr_1); diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index cfef572..c05dfeb 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3021,6 +3021,7 @@ SubXactCallback SubXactCallbackItem SubXactEvent SubXactInfo +SubplanNameSuffix SubqueryScan SubqueryScanPath SubqueryScanState -- 2.56.0
