From e6440d5007aaa33f946ab63f2c1dfd1ee99e2050 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 21 Apr 2022 17:58:52 -0400 Subject: [PATCH] Remove inadequate assertion check in CTE inlining. inline_cte() expected to find exactly as many references to the target CTE as its cterefcount indicates. While that should be accurate for the tree as emitted by the parser, there are some optimizations that occur upstream of here that could falsify it, notably removal of unused subquery output expressions. Trying to make the accounting 100% accurate seems expensive and doomed to future breakage. It's not really worth it, because all this code is protecting is downstream assumptions that every referenced CTE has a plan. Let's convert those assertions to regular test-and-elog just in case there's some actual problem, and then drop the failing assertion. Per report from Tomas Vondra (thanks also to Richard Guo for analysis). Back-patch to v12 where the faulty code came in. Discussion: https://postgr.es/m/29196a1e-ed47-c7ca-9be2-b1c636816183@enterprisedb.com --- src/backend/optimizer/path/allpaths.c | 3 +- src/backend/optimizer/plan/createplan.c | 3 +- src/backend/optimizer/plan/subselect.c | 8 ---- src/include/nodes/pathnodes.h | 3 +- src/test/regress/expected/with.out | 64 +++++++++++++++++++++++++ src/test/regress/sql/with.sql | 31 ++++++++++++ 6 files changed, 101 insertions(+), 11 deletions(-) diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index e7a3e92bc2..3723863a85 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -2538,7 +2538,8 @@ set_cte_pathlist(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte) if (ndx >= list_length(cteroot->cte_plan_ids)) elog(ERROR, "could not find plan for CTE \"%s\"", rte->ctename); plan_id = list_nth_int(cteroot->cte_plan_ids, ndx); - Assert(plan_id > 0); + if (plan_id <= 0) + elog(ERROR, "no plan was made for CTE \"%s\"", rte->ctename); cteplan = (Plan *) list_nth(root->glob->subplans, plan_id - 1); /* Mark rel with estimated output rows, width, etc */ diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index e445debe57..917713c163 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -3709,7 +3709,8 @@ create_ctescan_plan(PlannerInfo *root, Path *best_path, if (ndx >= list_length(cteroot->cte_plan_ids)) elog(ERROR, "could not find plan for CTE \"%s\"", rte->ctename); plan_id = list_nth_int(cteroot->cte_plan_ids, ndx); - Assert(plan_id > 0); + if (plan_id <= 0) + elog(ERROR, "no plan was made for CTE \"%s\"", rte->ctename); foreach(lc, cteroot->init_plans) { ctesplan = (SubPlan *) lfirst(lc); diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index 11e29dd153..91a8851b25 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -64,7 +64,6 @@ typedef struct inline_cte_walker_context { const char *ctename; /* name and relative level of target CTE */ int levelsup; - int refcount; /* number of remaining references */ Query *ctequery; /* query to substitute */ } inline_cte_walker_context; @@ -1131,13 +1130,9 @@ inline_cte(PlannerInfo *root, CommonTableExpr *cte) context.ctename = cte->ctename; /* Start at levelsup = -1 because we'll immediately increment it */ context.levelsup = -1; - context.refcount = cte->cterefcount; context.ctequery = castNode(Query, cte->ctequery); (void) inline_cte_walker((Node *) root->parse, &context); - - /* Assert we replaced all references */ - Assert(context.refcount == 0); } static bool @@ -1200,9 +1195,6 @@ inline_cte_walker(Node *node, inline_cte_walker_context *context) rte->coltypes = NIL; rte->coltypmods = NIL; rte->colcollations = NIL; - - /* Count the number of replacements we've done */ - context->refcount--; } return false; diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 69150e46eb..5ebf070979 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -258,7 +258,8 @@ struct PlannerInfo List *init_plans; /* init SubPlans for query */ - List *cte_plan_ids; /* per-CTE-item list of subplan IDs */ + List *cte_plan_ids; /* per-CTE-item list of subplan IDs (or -1 if + * no subplan was made for that CTE) */ List *multiexpr_params; /* List of Lists of Params for MULTIEXPR * subquery outputs */ diff --git a/src/test/regress/expected/with.out b/src/test/regress/expected/with.out index 04a942993d..63bbef0677 100644 --- a/src/test/regress/expected/with.out +++ b/src/test/regress/expected/with.out @@ -1766,6 +1766,70 @@ SELECT * FROM bug6051_3; --- (0 rows) +-- check case where CTE reference is removed due to optimization +EXPLAIN (VERBOSE, COSTS OFF) +SELECT q1 FROM +( + WITH t_cte AS (SELECT * FROM int8_tbl t) + SELECT q1, (SELECT q2 FROM t_cte WHERE t_cte.q1 = i8.q1) AS t_sub + FROM int8_tbl i8 +) ss; + QUERY PLAN +-------------------------------------- + Subquery Scan on ss + Output: ss.q1 + -> Seq Scan on public.int8_tbl i8 + Output: i8.q1, NULL::bigint +(4 rows) + +SELECT q1 FROM +( + WITH t_cte AS (SELECT * FROM int8_tbl t) + SELECT q1, (SELECT q2 FROM t_cte WHERE t_cte.q1 = i8.q1) AS t_sub + FROM int8_tbl i8 +) ss; + q1 +------------------ + 123 + 123 + 4567890123456789 + 4567890123456789 + 4567890123456789 +(5 rows) + +EXPLAIN (VERBOSE, COSTS OFF) +SELECT q1 FROM +( + WITH t_cte AS MATERIALIZED (SELECT * FROM int8_tbl t) + SELECT q1, (SELECT q2 FROM t_cte WHERE t_cte.q1 = i8.q1) AS t_sub + FROM int8_tbl i8 +) ss; + QUERY PLAN +--------------------------------------------- + Subquery Scan on ss + Output: ss.q1 + -> Seq Scan on public.int8_tbl i8 + Output: i8.q1, NULL::bigint + CTE t_cte + -> Seq Scan on public.int8_tbl t + Output: t.q1, t.q2 +(7 rows) + +SELECT q1 FROM +( + WITH t_cte AS MATERIALIZED (SELECT * FROM int8_tbl t) + SELECT q1, (SELECT q2 FROM t_cte WHERE t_cte.q1 = i8.q1) AS t_sub + FROM int8_tbl i8 +) ss; + q1 +------------------ + 123 + 123 + 4567890123456789 + 4567890123456789 + 4567890123456789 +(5 rows) + -- a truly recursive CTE in the same list WITH RECURSIVE t(a) AS ( SELECT 0 diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql index 75b310cac5..d2c4c174a4 100644 --- a/src/test/regress/sql/with.sql +++ b/src/test/regress/sql/with.sql @@ -842,6 +842,37 @@ COMMIT; SELECT * FROM bug6051_3; +-- check case where CTE reference is removed due to optimization +EXPLAIN (VERBOSE, COSTS OFF) +SELECT q1 FROM +( + WITH t_cte AS (SELECT * FROM int8_tbl t) + SELECT q1, (SELECT q2 FROM t_cte WHERE t_cte.q1 = i8.q1) AS t_sub + FROM int8_tbl i8 +) ss; + +SELECT q1 FROM +( + WITH t_cte AS (SELECT * FROM int8_tbl t) + SELECT q1, (SELECT q2 FROM t_cte WHERE t_cte.q1 = i8.q1) AS t_sub + FROM int8_tbl i8 +) ss; + +EXPLAIN (VERBOSE, COSTS OFF) +SELECT q1 FROM +( + WITH t_cte AS MATERIALIZED (SELECT * FROM int8_tbl t) + SELECT q1, (SELECT q2 FROM t_cte WHERE t_cte.q1 = i8.q1) AS t_sub + FROM int8_tbl i8 +) ss; + +SELECT q1 FROM +( + WITH t_cte AS MATERIALIZED (SELECT * FROM int8_tbl t) + SELECT q1, (SELECT q2 FROM t_cte WHERE t_cte.q1 = i8.q1) AS t_sub + FROM int8_tbl i8 +) ss; + -- a truly recursive CTE in the same list WITH RECURSIVE t(a) AS ( SELECT 0