Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oVbpj-0002E8-1V for pgsql-bugs@arkaria.postgresql.org; Tue, 06 Sep 2022 16:53:55 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1oVbph-0005Xf-43 for pgsql-bugs@arkaria.postgresql.org; Tue, 06 Sep 2022 16:53:53 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oVbpg-0005XW-PE for pgsql-bugs@lists.postgresql.org; Tue, 06 Sep 2022 16:53:52 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oVbpZ-0007Jq-Qq for pgsql-bugs@lists.postgresql.org; Tue, 06 Sep 2022 16:53:52 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 286Grffn3251632; Tue, 6 Sep 2022 12:53:41 -0400 From: Tom Lane To: Richard Guo cc: 857348270@qq.com, PostgreSQL mailing lists Subject: Re: BUG #17606: There is still some glitch in 3f7323cbb fixing failure of MULTIEXPR_SUBLINK In-reply-to: <2439275.1662394684@sss.pgh.pa.us> References: <17606-e5c8ad18d31db96a@postgresql.org> <2435363.1662393106@sss.pgh.pa.us> <2439275.1662394684@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Mon, 05 Sep 2022 12:18:04 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <3251543.1662483173.0@sss.pgh.pa.us> Date: Tue, 06 Sep 2022 12:53:41 -0400 Message-ID: <3251631.1662483221@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <3251543.1662483173.1@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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fix-multiexpr-renumbering-some-more.patch"; charset="us-ascii" Content-ID: <3251543.1662483173.2@sss.pgh.pa.us> Content-Description: fix-multiexpr-renumbering-some-more.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 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= .c index 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.c index 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.c index 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/optimize= r/plan/subselect.c index 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, PlannerIn= fo *subroot, */ splan =3D makeNode(SubPlan); splan->subLinkType =3D subLinkType; + splan->subLinkId =3D subLinkId; splan->testexpr =3D NULL; splan->paramIds =3D 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 P= aram * 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 ar= e + * initplans, because those don't have input parameters that could cause + * confusion. Such initplans will not appear in the targetlist anyway, b= ut + * they still complicate matters because the surviving regular subplans m= ight + * now not have consecutive subLinkIds. */ void SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot) { - List *new_multiexpr_params =3D NIL; - int offset; + List *param_mapping =3D NIL; ListCell *lc; = /* @@ -876,6 +883,9 @@ SS_make_multiexprs_unique(PlannerInfo *root, PlannerIn= fo *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, Planner= Info *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 =3D lappend(new_multiexpr_params, params); + root->multiexpr_params =3D lappend(root->multiexpr_params, params); + oldId =3D splan->subLinkId; + newId =3D list_length(root->multiexpr_params); + Assert(newId > oldId); + splan->subLinkId =3D 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) <=3D oldId) + param_mapping =3D lappend_int(param_mapping, 0); + lc2 =3D list_nth_cell(param_mapping, oldId); + lfirst_int(lc2) =3D 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 cou= ld - * 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 fi= nd + * 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 su= ch + * as FieldStores and SubscriptingRefs and type coercions. */ - offset =3D list_length(root->multiexpr_params); + if (param_mapping !=3D 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 nee= ded. + * (We can update-in-place because the tree was already copied.) + */ +static bool +SS_make_multiexprs_unique_walker(Node *node, void *context) +{ + if (node =3D=3D NULL) + return false; + if (IsA(node, Param)) { - TargetEntry *tent =3D (TargetEntry *) lfirst(lc); - Node *expr; - Param *p; + Param *p =3D (Param *) node; + List *param_mapping =3D (List *) context; int subqueryid; int colno; + int newId; = - expr =3D (Node *) tent->expr; - while (expr) - { - if (IsA(expr, FieldStore)) - { - FieldStore *fstore =3D (FieldStore *) expr; - - expr =3D (Node *) linitial(fstore->newvals); - } - else if (IsA(expr, SubscriptingRef)) - { - SubscriptingRef *sbsref =3D (SubscriptingRef *) expr; - - if (sbsref->refassgnexpr =3D=3D NULL) - break; - - expr =3D (Node *) sbsref->refassgnexpr; - } - else if (IsA(expr, CoerceToDomain)) - { - CoerceToDomain *cdomain =3D (CoerceToDomain *) expr; - - if (cdomain->coercionformat !=3D COERCE_IMPLICIT_CAST) - break; - expr =3D (Node *) cdomain->arg; - } - else - break; - } - expr =3D strip_implicit_coercions(expr); - if (expr =3D=3D NULL || !IsA(expr, Param)) - continue; - p =3D (Param *) expr; if (p->paramkind !=3D PARAM_MULTIEXPR) - continue; + return false; subqueryid =3D p->paramid >> 16; colno =3D p->paramid & 0xFFFF; - Assert(subqueryid > 0 && - subqueryid <=3D list_length(new_multiexpr_params)); - Assert(colno > 0 && - colno <=3D list_length((List *) list_nth(new_multiexpr_params, - subqueryid - 1))); - subqueryid +=3D offset; - p->paramid =3D (subqueryid << 16) + colno; - } = - /* Finally, attach new replacement lists to the global list */ - root->multiexpr_params =3D 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 >=3D list_length(param_mapping)) + return false; + newId =3D list_nth_int(param_mapping, subqueryid); + if (newId =3D=3D 0) + return false; + p->paramid =3D (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 =3D makeNode(SubPlan); splan->subLinkType =3D CTE_SUBLINK; + splan->subLinkId =3D 0; splan->testexpr =3D NULL; splan->paramIds =3D NIL; get_first_col_type(plan, &splan->firstColType, &splan->firstColTypmod, @@ -3075,6 +3077,7 @@ SS_make_initplan_from_plan(PlannerInfo *root, */ node =3D makeNode(SubPlan); node->subLinkType =3D EXPR_SUBLINK; + node->subLinkId =3D 0; node->plan_id =3D list_length(root->glob->subplans); node->plan_name =3D psprintf("InitPlan %d (returns $%d)", node->plan_id, prm->paramid); diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index 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/expe= cted/inherit.out index 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]) =3D (select p2.unique2, p2.stringu1 - from int4_tbl limit 1) +update inhpar set + (f1, f2[1]) =3D (select p2.unique2, p2.stringu1 from int4_tbl limit = 1), + (f2[2], f2[3]) =3D (select 'x', 'y' from int4_tbl limit 1), + (f3, f2[4]) =3D (select p2.unique2, p2.stringu1 from int4_tbl limit = 1), + (f2[5], f2[6]) =3D (select 'x', 'y' from int4_tbl limit 1) from onek p2 where inhpar.f1 =3D 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] :=3D $5, (SubPlan 1 (returns $2,$3)), i= nhpar.ctid, p2.ctid + Output: $12, (((((inhpar.f2[1] :=3D $13)[2] :=3D $4)[3] :=3D $5)= [4] :=3D $15)[5] :=3D $10)[6] :=3D $11, $14, (SubPlan 1 (returns $2,$3)), = NULL::record, (SubPlan 3 (returns $8,$9)), NULL::record, inhpar.ctid, p2.c= tid Merge Cond: (p2.unique1 =3D 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 =3D 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] :=3D $7, (SubPlan 1 (returns $2,$3)),= inhpar_1.ctid, p2.ctid + Output: $16, (((((inhpar_1.f2[1] :=3D $17)[2] :=3D $4)[3] :=3D $= 5)[4] :=3D $19)[5] :=3D $10)[6] :=3D $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 =3D 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 =3D 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]) =3D (select p2.unique2, p2.stringu1 - from int4_tbl limit 1) +update inhpar set + (f1, f2[1]) =3D (select p2.unique2, p2.stringu1 from int4_tbl limit = 1), + (f2[2], f2[3]) =3D (select 'x', 'y' from int4_tbl limit 1), + (f3, f2[4]) =3D (select p2.unique2, p2.stringu1 from int4_tbl limit = 1), + (f2[5], f2[6]) =3D (select 'x', 'y' from int4_tbl limit 1) from onek p2 where inhpar.f1 =3D 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/inher= it.sql index 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]) =3D (select p2.unique2, p2.stringu1 - from int4_tbl limit 1) +update inhpar set + (f1, f2[1]) =3D (select p2.unique2, p2.stringu1 from int4_tbl limit = 1), + (f2[2], f2[3]) =3D (select 'x', 'y' from int4_tbl limit 1), + (f3, f2[4]) =3D (select p2.unique2, p2.stringu1 from int4_tbl limit = 1), + (f2[5], f2[6]) =3D (select 'x', 'y' from int4_tbl limit 1) from onek p2 where inhpar.f1 =3D p2.unique1; -update inhpar set (f1, f2[1]) =3D (select p2.unique2, p2.stringu1 - from int4_tbl limit 1) +update inhpar set + (f1, f2[1]) =3D (select p2.unique2, p2.stringu1 from int4_tbl limit = 1), + (f2[2], f2[3]) =3D (select 'x', 'y' from int4_tbl limit 1), + (f3, f2[4]) =3D (select p2.unique2, p2.stringu1 from int4_tbl limit = 1), + (f2[5], f2[6]) =3D (select 'x', 'y' from int4_tbl limit 1) from onek p2 where inhpar.f1 =3D p2.unique1; = drop table inhpar cascade; ------- =_aaaaaaaaaa0--