diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c index fadd8ea731..c542464e5a 100644 --- a/src/backend/executor/nodeSubplan.c +++ b/src/backend/executor/nodeSubplan.c @@ -245,6 +245,21 @@ ExecScanSubPlan(SubPlanState *node, * ones, so this should be safe.) Unlike ExecReScanSetParamPlan, we do * *not* set bits in the parent plan node's chgParam, because we don't * want to cause a rescan of the parent. + * + * Note: we are also relying on MULTIEXPR SubPlans not sharing any output + * parameters with other SubPlans, because if one does then it is unclear + * which SubPlanState node the parameter's execPlan field will be pointing + * to when we come to evaluate the parameter. We can allow plain initplan + * SubPlans to share output parameters, because it doesn't actually matter + * which initplan SubPlan we reference as long as they all point to the + * same underlying subplan. However, that fails to hold for MULTIEXPRs + * because they can have non-empty args lists, and the "same" args might + * have mutated into different forms in different parts of a plan tree. + * There is not a problem in ordinary queries because MULTIEXPR will + * appear only in an UPDATE's top-level target list, so it won't get + * duplicated anyplace. However, when inheritance_planner clones a + * partially-planned targetlist it must take care to assign non-duplicate + * param IDs to the cloned copy. */ if (subLinkType == MULTIEXPR_SUBLINK) { diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 60e7fda6a9..27c665ac12 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -1606,6 +1606,10 @@ inheritance_planner(PlannerInfo *root) /* and we haven't created PlaceHolderInfos, either */ Assert(subroot->placeholder_list == NIL); + /* Fix MULTIEXPR_SUBLINK params if any */ + if (root->multiexpr_params) + SS_make_multiexprs_unique(root, subroot); + /* Generate Path(s) for accessing this result relation */ grouping_planner(subroot, true, 0.0 /* retrieve all tuples */ ); diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index 91a8851b25..927d6a43bd 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -851,6 +851,101 @@ hash_ok_operator(OpExpr *expr) } } +/* + * SS_make_multiexprs_unique + * + * After cloning an UPDATE targetlist that contains MULTIEXPR_SUBLINK + * SubPlans, inheritance_planner() must call this to assign new, unique Param + * IDs to the cloned MULTIEXPR_SUBLINKs' output parameters. See notes in + * ExecScanSubPlan. + */ +void +SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot) +{ + List *new_multiexpr_params = NIL; + int offset; + ListCell *lc; + + /* + * Find MULTIEXPR SubPlans in the cloned query. We need only look at the + * top level of the targetlist. + */ + foreach(lc, subroot->parse->targetList) + { + TargetEntry *tent = (TargetEntry *) lfirst(lc); + SubPlan *splan; + Plan *plan; + List *params; + + if (!IsA(tent->expr, SubPlan)) + continue; + splan = (SubPlan *) tent->expr; + if (splan->subLinkType != MULTIEXPR_SUBLINK) + continue; + + /* Found one, get the associated subplan */ + plan = (Plan *) list_nth(root->glob->subplans, splan->plan_id - 1); + + /* + * Generate new PARAM_EXEC Param nodes, and overwrite splan->setParam + * with their IDs. This is just like what build_subplan did when it + * made the SubPlan node we're cloning. But because the param IDs are + * assigned globally, we'll get new IDs. (We assume here that the + * subroot's tlist is a clone we can scribble on.) + */ + params = generate_subquery_params(root, + plan->targetlist, + &splan->setParam); + + /* + * We will append the replacement-Params lists to + * root->multiexpr_params, but for the moment just make a local list. + * Since we lack easy access here to the original subLinkId, we have + * to fall back on the slightly shaky assumption that the MULTIEXPR + * SubPlans appear in the targetlist in subLinkId order. This should + * be safe enough given the way that the parser builds the targetlist + * today. I wouldn't want to rely on it going forward, but since this + * code has a limited lifespan it should be fine. We can partially + * protect against problems with assertions below. + */ + new_multiexpr_params = lappend(new_multiexpr_params, params); + } + + /* + * Now we must find the Param nodes that reference the MULTIEXPR outputs + * and update their sublink IDs so they'll reference the new outputs. + * Fortunately, those too must be at top level of the cloned targetlist. + */ + offset = list_length(root->multiexpr_params); + + foreach(lc, subroot->parse->targetList) + { + TargetEntry *tent = (TargetEntry *) lfirst(lc); + Param *p; + int subqueryid; + int colno; + + if (!IsA(tent->expr, Param)) + continue; + p = (Param *) tent->expr; + if (p->paramkind != PARAM_MULTIEXPR) + continue; + subqueryid = p->paramid >> 16; + colno = p->paramid & 0xFFFF; + Assert(subqueryid > 0 && + subqueryid <= list_length(new_multiexpr_params)); + Assert(colno > 0 && + colno <= list_length((List *) list_nth(new_multiexpr_params, + subqueryid - 1))); + subqueryid += offset; + p->paramid = (subqueryid << 16) + colno; + } + + /* Finally, attach new replacement lists to the global list */ + root->multiexpr_params = list_concat(root->multiexpr_params, + new_multiexpr_params); +} + /* * SS_process_ctes: process a query's WITH list diff --git a/src/include/optimizer/subselect.h b/src/include/optimizer/subselect.h index d6a872bd2c..6f33578c8f 100644 --- a/src/include/optimizer/subselect.h +++ b/src/include/optimizer/subselect.h @@ -16,6 +16,7 @@ #include "nodes/pathnodes.h" #include "nodes/plannodes.h" +extern void SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot); extern void SS_process_ctes(PlannerInfo *root); extern JoinExpr *convert_ANY_sublink_to_join(PlannerInfo *root, SubLink *sublink, diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index 2b68aef654..6e134d53f6 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -1715,6 +1715,55 @@ reset enable_seqscan; reset enable_indexscan; reset enable_bitmapscan; -- +-- Check handling of MULTIEXPR SubPlans in inherited updates +-- +create table inhpar(f1 int, f2 name); +insert into inhpar select generate_series(1,10); +create table inhcld() inherits(inhpar); +insert into inhcld select generate_series(11,10000); +vacuum analyze inhcld; +vacuum analyze inhpar; +explain (verbose, costs off) +update inhpar set (f1, f2) = (select p2.unique2, p2.stringu1 + from int4_tbl limit 1) +from onek p2 where inhpar.f1 = p2.unique1; + QUERY PLAN +----------------------------------------------------------------------------- + Update on public.inhpar + Update on public.inhpar + Update on public.inhcld inhpar_1 + -> Merge Join + Output: $4, $5, (SubPlan 1 (returns $2,$3)), inhpar.ctid, p2.ctid + Merge Cond: (p2.unique1 = inhpar.f1) + -> Index Scan using onek_unique1 on public.onek p2 + Output: p2.unique2, p2.stringu1, p2.ctid, p2.unique1 + -> Sort + Output: inhpar.ctid, inhpar.f1 + Sort Key: inhpar.f1 + -> Seq Scan on public.inhpar + Output: inhpar.ctid, inhpar.f1 + SubPlan 1 (returns $2,$3) + -> Limit + Output: (p2.unique2), (p2.stringu1) + -> Seq Scan on public.int4_tbl + Output: p2.unique2, p2.stringu1 + -> Hash Join + Output: $6, $7, (SubPlan 1 (returns $2,$3)), inhpar_1.ctid, p2.ctid + Hash Cond: (inhpar_1.f1 = p2.unique1) + -> Seq Scan on public.inhcld inhpar_1 + Output: inhpar_1.ctid, inhpar_1.f1 + -> Hash + Output: p2.unique2, p2.stringu1, p2.ctid, p2.unique1 + -> Seq Scan on public.onek p2 + Output: p2.unique2, p2.stringu1, p2.ctid, p2.unique1 +(27 rows) + +update inhpar set (f1, f2) = (select p2.unique2, p2.stringu1 + from int4_tbl limit 1) +from onek p2 where inhpar.f1 = p2.unique1; +drop table inhpar cascade; +NOTICE: drop cascades to table inhcld +-- -- Check handling of a constant-null CHECK constraint -- create table cnullparent (f1 int); diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index 64173a8738..fd4f252c29 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -629,6 +629,26 @@ reset enable_seqscan; reset enable_indexscan; reset enable_bitmapscan; +-- +-- Check handling of MULTIEXPR SubPlans in inherited updates +-- +create table inhpar(f1 int, f2 name); +insert into inhpar select generate_series(1,10); +create table inhcld() inherits(inhpar); +insert into inhcld select generate_series(11,10000); +vacuum analyze inhcld; +vacuum analyze inhpar; + +explain (verbose, costs off) +update inhpar set (f1, f2) = (select p2.unique2, p2.stringu1 + from int4_tbl limit 1) +from onek p2 where inhpar.f1 = p2.unique1; +update inhpar set (f1, f2) = (select p2.unique2, p2.stringu1 + from int4_tbl limit 1) +from onek p2 where inhpar.f1 = p2.unique1; + +drop table inhpar cascade; + -- -- Check handling of a constant-null CHECK constraint --