From: Tom Lane <tgl@sss.pgh.pa.us>
To: Richard Guo <guofenglinux@gmail.com>
Cc: 857348270@qq.com, PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #17606: There is still some glitch in 3f7323cbb fixing failure of MULTIEXPR_SUBLINK
Date: Tue, 06 Sep 2022 12:53:41 -0400
Message-ID: <3251631.1662483221@sss.pgh.pa.us> (raw)
In-Reply-To: <2439275.1662394684@sss.pgh.pa.us>
References: <17606-e5c8ad18d31db96a@postgresql.org>
<CAMbWs49A0FwQKq=_3pn6tRJqG5E_Y4uiUj9zAtuNbS0cTMYZ_A@mail.gmail.com>
<CAMbWs48Qf=2ZL-d-0=9rYBZUPsp6FdLysuYULTtEvErwi3w3jg@mail.gmail.com>
<2435363.1662393106@sss.pgh.pa.us>
<2439275.1662394684@sss.pgh.pa.us>
I wrote:
> After contemplating that for awhile, by far the least painful way
> seems to be to add the subLinkId to struct SubPlan. We can get
> away with that in the back branches as long as we add it at the
> end, since SubPlans don't get recorded in the catalogs.
The attached seems to fix it. While writing a test case I discovered
that the code I'd stolen from ruleutils.c is inadequate for the purpose.
That code was only designed to deal with pre-planning query trees,
so it only expects one Param per targetlist entry --- but cases like
multiple assignments to the same array column can get rolled up into
nests containing several Params (cf. rewriteTargetListIU). So I abandoned
that tactic in favor of just writing a tree walker. Andre's gut feeling
that we needed one was right.
regards, tom lane
Attachments:fix-multiexpr-renumbering-some-more.patch (text/x-diff, 15.3K) downloaddiff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.cindex 8db09fda63..692b6c1559 100644--- a/src/backend/nodes/copyfuncs.c+++ b/src/backend/nodes/copyfuncs.c@@ -1719,6 +1719,7 @@ _copySubPlan(const SubPlan *from)
COPY_NODE_FIELD(args);
COPY_SCALAR_FIELD(startup_cost);
COPY_SCALAR_FIELD(per_call_cost);
+ COPY_SCALAR_FIELD(subLinkId);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.cindex 092fb37923..337be2f896 100644--- a/src/backend/nodes/equalfuncs.c+++ b/src/backend/nodes/equalfuncs.c@@ -455,6 +455,7 @@ _equalSubPlan(const SubPlan *a, const SubPlan *b)
COMPARE_NODE_FIELD(args);
COMPARE_SCALAR_FIELD(startup_cost);
COMPARE_SCALAR_FIELD(per_call_cost);
+ COMPARE_SCALAR_FIELD(subLinkId);
return true;
}
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.cindex 4547eed43b..21ececf0c2 100644--- a/src/backend/nodes/outfuncs.c+++ b/src/backend/nodes/outfuncs.c@@ -1349,6 +1349,7 @@ _outSubPlan(StringInfo str, const SubPlan *node)
WRITE_NODE_FIELD(args);
WRITE_FLOAT_FIELD(startup_cost, "%.2f");
WRITE_FLOAT_FIELD(per_call_cost, "%.2f");
+ WRITE_INT_FIELD(subLinkId);
}
static void
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.cindex 62c945b6c5..7976b369ba 100644--- a/src/backend/nodes/readfuncs.c+++ b/src/backend/nodes/readfuncs.c@@ -2536,6 +2536,7 @@ _readSubPlan(void)
READ_NODE_FIELD(args);
READ_FLOAT_FIELD(startup_cost);
READ_FLOAT_FIELD(per_call_cost);
+ READ_INT_FIELD(subLinkId);
READ_DONE();
}
diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.cindex a873d72b3a..7a4c85faf2 100644--- a/src/backend/optimizer/plan/subselect.c+++ b/src/backend/optimizer/plan/subselect.c@@ -86,6 +86,7 @@ static bool subplan_is_hashable(Plan *plan);
static bool testexpr_is_hashable(Node *testexpr, List *param_ids);
static bool test_opexpr_is_hashable(OpExpr *testexpr, List *param_ids);
static bool hash_ok_operator(OpExpr *expr);
+static bool SS_make_multiexprs_unique_walker(Node *node, void *context);
static bool contain_dml(Node *node);
static bool contain_dml_walker(Node *node, void *context);
static bool contain_outer_selfref(Node *node);
@@ -335,6 +336,7 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
*/
splan = makeNode(SubPlan);
splan->subLinkType = subLinkType;
+ splan->subLinkId = subLinkId;
splan->testexpr = NULL;
splan->paramIds = NIL;
get_first_col_type(plan, &splan->firstColType, &splan->firstColTypmod,
@@ -858,12 +860,17 @@ hash_ok_operator(OpExpr *expr)
* SubPlans, inheritance_planner() must call this to assign new, unique Param
* IDs to the cloned MULTIEXPR_SUBLINKs' output parameters. See notes in
* ExecScanSubPlan.
+ *+ * We do not need to re-map Param IDs for MULTIEXPR_SUBLINK plans that are+ * initplans, because those don't have input parameters that could cause+ * confusion. Such initplans will not appear in the targetlist anyway, but+ * they still complicate matters because the surviving regular subplans might+ * now not have consecutive subLinkIds.
*/
void
SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot)
{
- List *new_multiexpr_params = NIL;- int offset;+ List *param_mapping = NIL;
ListCell *lc;
/*
@@ -876,6 +883,9 @@ SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot)
SubPlan *splan;
Plan *plan;
List *params;
+ int oldId,+ newId;+ ListCell *lc2;
if (!IsA(tent->expr, SubPlan))
continue;
@@ -898,86 +908,77 @@ SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot)
&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.+ * Append the new replacement-Params list to root->multiexpr_params.+ * Then its index in that list becomes the new subPlanId of the+ * SubPlan.
*/
- new_multiexpr_params = lappend(new_multiexpr_params, params);+ root->multiexpr_params = lappend(root->multiexpr_params, params);+ oldId = splan->subLinkId;+ newId = list_length(root->multiexpr_params);+ Assert(newId > oldId);+ splan->subLinkId = newId;++ /*+ * Add a mapping entry to param_mapping so that we can update the+ * associated Params below. Leave zeroes in the list for any+ * subLinkIds we don't encounter; those must have been converted to+ * initplans.+ */+ while (list_length(param_mapping) <= oldId)+ param_mapping = lappend_int(param_mapping, 0);+ lc2 = list_nth_cell(param_mapping, oldId);+ lfirst_int(lc2) = newId;
}
/*
- * 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 in the cloned targetlist, but they could- * be buried under FieldStores and SubscriptingRefs and CoerceToDomains- * (cf processIndirection()), and underneath those there could be an- * implicit type coercion.+ * Unless all the MULTIEXPRs were converted to initplans, we must now find+ * the Param nodes that reference the MULTIEXPR outputs and update their+ * sublink IDs so they'll reference the new outputs. While such Params+ * must be in the cloned targetlist, they could be buried under stuff such+ * as FieldStores and SubscriptingRefs and type coercions.
*/
- offset = list_length(root->multiexpr_params);+ if (param_mapping != NIL)+ SS_make_multiexprs_unique_walker((Node *) subroot->parse->targetList,+ (void *) param_mapping);+}- foreach(lc, subroot->parse->targetList)+/*+ * Locate PARAM_MULTIEXPR Params in an expression tree, and update as needed.+ * (We can update-in-place because the tree was already copied.)+ */+static bool+SS_make_multiexprs_unique_walker(Node *node, void *context)+{+ if (node == NULL)+ return false;+ if (IsA(node, Param))
{
- TargetEntry *tent = (TargetEntry *) lfirst(lc);- Node *expr;- Param *p;+ Param *p = (Param *) node;+ List *param_mapping = (List *) context;
int subqueryid;
int colno;
+ int newId;- expr = (Node *) tent->expr;- while (expr)- {- if (IsA(expr, FieldStore))- {- FieldStore *fstore = (FieldStore *) expr;-- expr = (Node *) linitial(fstore->newvals);- }- else if (IsA(expr, SubscriptingRef))- {- SubscriptingRef *sbsref = (SubscriptingRef *) expr;-- if (sbsref->refassgnexpr == NULL)- break;-- expr = (Node *) sbsref->refassgnexpr;- }- else if (IsA(expr, CoerceToDomain))- {- CoerceToDomain *cdomain = (CoerceToDomain *) expr;-- if (cdomain->coercionformat != COERCE_IMPLICIT_CAST)- break;- expr = (Node *) cdomain->arg;- }- else- break;- }- expr = strip_implicit_coercions(expr);- if (expr == NULL || !IsA(expr, Param))- continue;- p = (Param *) expr;
if (p->paramkind != PARAM_MULTIEXPR)
- continue;+ return false;
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);+ /*+ * If subqueryid doesn't have a mapping entry, it must refer to an+ * initplan, so don't change the Param.+ */+ Assert(subqueryid > 0);+ if (subqueryid >= list_length(param_mapping))+ return false;+ newId = list_nth_int(param_mapping, subqueryid);+ if (newId == 0)+ return false;+ p->paramid = (newId << 16) + colno;+ return false;+ }+ return expression_tree_walker(node, SS_make_multiexprs_unique_walker,+ context);
}
@@ -1110,6 +1111,7 @@ SS_process_ctes(PlannerInfo *root)
*/
splan = makeNode(SubPlan);
splan->subLinkType = CTE_SUBLINK;
+ splan->subLinkId = 0;
splan->testexpr = NULL;
splan->paramIds = NIL;
get_first_col_type(plan, &splan->firstColType, &splan->firstColTypmod,
@@ -3075,6 +3077,7 @@ SS_make_initplan_from_plan(PlannerInfo *root,
*/
node = makeNode(SubPlan);
node->subLinkType = EXPR_SUBLINK;
+ node->subLinkId = 0;
node->plan_id = list_length(root->glob->subplans);
node->plan_name = psprintf("InitPlan %d (returns $%d)",
node->plan_id, prm->paramid);
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.hindex d73be2ad46..cfc4f5a9a7 100644--- a/src/include/nodes/primnodes.h+++ b/src/include/nodes/primnodes.h@@ -731,6 +731,8 @@ typedef struct SubPlan
/* Estimated execution costs: */
Cost startup_cost; /* one-time setup cost */
Cost per_call_cost; /* cost for each subplan evaluation */
+ /* Copied from original SubLink, but placed at end for ABI stability */+ int subLinkId; /* ID (1..n); 0 if not MULTIEXPR */
} SubPlan;
/*
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.outindex ed99bdde7f..658fed7922 100644--- a/src/test/regress/expected/inherit.out+++ b/src/test/regress/expected/inherit.out@@ -1717,23 +1717,36 @@ reset enable_bitmapscan;
--
-- Check handling of MULTIEXPR SubPlans in inherited updates
--
-create table inhpar(f1 int, f2 text[]);+create table inhpar(f1 int, f2 text[], f3 int);
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[1]) = (select p2.unique2, p2.stringu1- from int4_tbl limit 1)+update inhpar set+ (f1, f2[1]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),+ (f2[2], f2[3]) = (select 'x', 'y' from int4_tbl limit 1),+ (f3, f2[4]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),+ (f2[5], f2[6]) = (select 'x', 'y' from int4_tbl limit 1)
from onek p2 where inhpar.f1 = p2.unique1;
- QUERY PLAN ------------------------------------------------------------------------------------------------+ QUERY PLAN +-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Update on public.inhpar
Update on public.inhpar
Update on public.inhcld inhpar_1
+ InitPlan 2 (returns $4,$5)+ -> Limit+ Output: 'x'::text, 'y'::text+ -> Seq Scan on public.int4_tbl int4_tbl_1+ Output: 'x'::text, 'y'::text+ InitPlan 4 (returns $10,$11)+ -> Limit+ Output: 'x'::text, 'y'::text+ -> Seq Scan on public.int4_tbl int4_tbl_3+ Output: 'x'::text, 'y'::text
-> Merge Join
- Output: $4, inhpar.f2[1] := $5, (SubPlan 1 (returns $2,$3)), inhpar.ctid, p2.ctid+ Output: $12, (((((inhpar.f2[1] := $13)[2] := $4)[3] := $5)[4] := $15)[5] := $10)[6] := $11, $14, (SubPlan 1 (returns $2,$3)), NULL::record, (SubPlan 3 (returns $8,$9)), NULL::record, 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
@@ -1747,8 +1760,13 @@ from onek p2 where inhpar.f1 = p2.unique1;
Output: (p2.unique2), (p2.stringu1)
-> Seq Scan on public.int4_tbl
Output: p2.unique2, p2.stringu1
+ SubPlan 3 (returns $8,$9)+ -> Limit+ Output: (p2.unique2), (p2.stringu1)+ -> Seq Scan on public.int4_tbl int4_tbl_2+ Output: p2.unique2, p2.stringu1
-> Hash Join
- Output: $6, inhpar_1.f2[1] := $7, (SubPlan 1 (returns $2,$3)), inhpar_1.ctid, p2.ctid+ Output: $16, (((((inhpar_1.f2[1] := $17)[2] := $4)[3] := $5)[4] := $19)[5] := $10)[6] := $11, $18, (SubPlan 1 (returns $2,$3)), NULL::record, (SubPlan 3 (returns $8,$9)), NULL::record, inhpar_1.ctid, p2.ctid
Hash Cond: (inhpar_1.f1 = p2.unique1)
-> Seq Scan on public.inhcld inhpar_1
Output: inhpar_1.f2, inhpar_1.ctid, inhpar_1.f1
@@ -1756,10 +1774,13 @@ from onek p2 where inhpar.f1 = p2.unique1;
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)+(42 rows)-update inhpar set (f1, f2[1]) = (select p2.unique2, p2.stringu1- from int4_tbl limit 1)+update inhpar set+ (f1, f2[1]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),+ (f2[2], f2[3]) = (select 'x', 'y' from int4_tbl limit 1),+ (f3, f2[4]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),+ (f2[5], f2[6]) = (select 'x', 'y' from int4_tbl limit 1)
from onek p2 where inhpar.f1 = p2.unique1;
drop table inhpar cascade;
NOTICE: drop cascades to table inhcld
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sqlindex 7d3813704e..a4a33a3da8 100644--- a/src/test/regress/sql/inherit.sql+++ b/src/test/regress/sql/inherit.sql@@ -632,7 +632,7 @@ reset enable_bitmapscan;
--
-- Check handling of MULTIEXPR SubPlans in inherited updates
--
-create table inhpar(f1 int, f2 text[]);+create table inhpar(f1 int, f2 text[], f3 int);
insert into inhpar select generate_series(1,10);
create table inhcld() inherits(inhpar);
insert into inhcld select generate_series(11,10000);
@@ -640,11 +640,17 @@ vacuum analyze inhcld;
vacuum analyze inhpar;
explain (verbose, costs off)
-update inhpar set (f1, f2[1]) = (select p2.unique2, p2.stringu1- from int4_tbl limit 1)+update inhpar set+ (f1, f2[1]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),+ (f2[2], f2[3]) = (select 'x', 'y' from int4_tbl limit 1),+ (f3, f2[4]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),+ (f2[5], f2[6]) = (select 'x', 'y' from int4_tbl limit 1)
from onek p2 where inhpar.f1 = p2.unique1;
-update inhpar set (f1, f2[1]) = (select p2.unique2, p2.stringu1- from int4_tbl limit 1)+update inhpar set+ (f1, f2[1]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),+ (f2[2], f2[3]) = (select 'x', 'y' from int4_tbl limit 1),+ (f3, f2[4]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),+ (f2[5], f2[6]) = (select 'x', 'y' from int4_tbl limit 1)
from onek p2 where inhpar.f1 = p2.unique1;
drop table inhpar cascade;
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, guofenglinux@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17606: There is still some glitch in 3f7323cbb fixing failure of MULTIEXPR_SUBLINK
In-Reply-To: <3251631.1662483221@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox