8000 Further fixes for MULTIEXPR_SUBLINK fix. · postgres/postgres@9bcf6fb · GitHub
[go: up one dir, main page]
Skip to content

Commit 9bcf6fb

Browse files
tglsfdctglsfdc
committed
Further fixes for MULTIEXPR_SUBLINK fix.
Some more things I didn't think about in commits 3f7323c et al: MULTIEXPR_SUBLINK subplans might have been converted to initplans instead of regular subplans, in which case they won't show up in the modified targetlist. Fortunately, this would only happen if they have no input parameters, which means that the problem we originally needed to fix can't happen with them. Therefore, there's no need to clone their output parameters, and thus it doesn't hurt that we'll fail to see them in the first pass over the targetlist. Nonetheless, this complicates matters greatly, because now we have to distinguish output Params of initplans (which shouldn't get renumbered) from those of regular subplans (which should). This also breaks the simplistic scheme I used of assuming that the subplans found in the targetlist have consecutive subLinkIds. We really can't avoid the need to know the subplans' subLinkIds in this code. To fix that, add subLinkId as the last field of SubPlan. We can get away with that change in back branches because SubPlan nodes will never be stored in the catalogs, and there's no ABI break for external code that might be looking at the existing fields of SubPlan. Secondly, rewriteTargetListIU might have rolled up multiple FieldStores or SubscriptingRefs into one targetlist entry, breaking the assumption that there's at most one Param to fix per targetlist entry. (That assumption is OK I think in the ruleutils.c code I stole the logic from in 18f5108, because that only deals with pre-rewrite query trees. But it's definitely not OK here.) Abandon that shortcut and just do a full tree walk on the targetlist to ensure we find all the Params we have to change. Per bug #17606 from Andre Lin. As before, only v10-v13 need the patch. Discussion: https://postgr.es/m/17606-e5c8ad18d31db96a@postgresql.org
1 parent a228cca commit 9bcf6fb

File tree

8 files changed

+119
-83
lines changed

8 files changed

+119
-83
lines changed

‎src/backend/nodes/copyfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1670,6 +1670,7 @@ _copySubPlan(const SubPlan *from)
16701670
COPY_NODE_FIELD(args);
16711671
COPY_SCALAR_FIELD(startup_cost);
16721672
COPY_SCALAR_FIELD(per_call_cost);
1673+
COPY_SCALAR_FIELD(subLinkId);
16731674

16741675
return newnode;
16751676
}

‎src/backend/nodes/equalfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,7 @@ _equalSubPlan(const SubPlan *a, const SubPlan *b)
450450
COMPARE_NODE_FIELD(args);
451451
COMPARE_SCALAR_FIELD(startup_cost);
452452
COMPARE_SCALAR_FIELD(per_call_cost);
453+
COMPARE_SCALAR_FIELD(subLinkId);
453454

454455
return true;
455456
}

‎src/backend/nodes/outfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1417,6 +1417,7 @@ _outSubPlan(StringInfo str, const SubPlan *node)
14171417
WRITE_NODE_FIELD(args);
14181418
WRITE_FLOAT_FIELD(startup_cost, "%.2f");
14191419
WRITE_FLOAT_FIELD(per_call_cost, "%.2f");
1420+
WRITE_INT_FIELD(subLinkId);
14201421
}
14211422

14221423
static void

‎src/backend/nodes/readfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2446,6 +2446,7 @@ _readSubPlan(void)
24462446
READ_NODE_FIELD(args);
24472447
READ_FLOAT_FIELD(startup_cost);
24482448
READ_FLOAT_FIELD(per_call_cost);
2449+
READ_INT_FIELD(subLinkId);
24492450

24502451
READ_DONE();
24512452
}

‎src/backend/optimizer/plan/subselect.c

Lines changed: 71 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ static bool subplan_is_hashable(Plan *plan);
7676
static bool testexpr_is_hashable(Node *testexpr, List *param_ids);
7777
static bool test_opexpr_is_hashable(OpExpr *testexpr, List *param_ids);
7878
static bool hash_ok_operator(OpExpr *expr);
79+
static bool SS_make_multiexprs_unique_walker(Node *node, void *context);
7980
static bool simplify_EXISTS_query(PlannerInfo *root, Query *query);
8081
static Query *convert_EXISTS_to_ANY(PlannerInfo *root, Query *subselect,
8182
Node **testexpr, List **paramIds);
@@ -336,6 +337,7 @@ build_subplan(PlannerInfo *root, Plan *plan, PlannerInfo *subroot,
336337
*/
337338
splan = makeNode(SubPlan);
338339
splan->subLinkType = subLinkType;
340+
splan->subLinkId = subLinkId;
339341
splan->testexpr = NULL;
340342
splan->paramIds = NIL;
341343
get_first_col_type(plan, &splan->firstColType, &splan->firstColTypmod,
@@ -859,12 +861,17 @@ hash_ok_operator(OpExpr *expr)
859861
* SubPlans, inheritance_planner() must call this to assign new, unique Param
860862
* IDs to the cloned MULTIEXPR_SUBLINKs' output parameters. See notes in
861863
* ExecScanSubPlan.
864+
*
865+
* We do not need to renumber Param IDs for MULTIEXPR_SUBLINK plans that are
866+
* initplans, because those don't have input parameters that could cause
867+
* confusion. Such initplans will not appear in the targetlist anyway, but
868+
* they still complicate matters because the surviving regular subplans might
869+
* not have consecutive subLinkIds.
862870
*/
863871
void
864872
SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot)
865873
{
866-
List *new_multiexpr_params = NIL;
867-
int offset;
874+
List *param_mapping = NIL;
868875
ListCell *lc;
869876

870877
/*
@@ -877,6 +884,9 @@ SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot)
877884
SubPlan *splan;
878885
Plan *plan;
879886
List *params;
887+
int oldId,
888+
newId;
889+
ListCell *lc2;
880890

881891
if (!IsA(tent->expr, SubPlan))
882892
continue;
@@ -899,86 +909,77 @@ SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot)
899909
&splan->setParam);
900910

901911
/*
902-
* We will append the replacement-Params lists to
903-
* root->multiexpr_params, but for the moment just make a local list.
904-
* Since we lack easy access here to the original subLinkId, we have
905-
* to fall back on the slightly shaky assumption that the MULTIEXPR
906-
* SubPlans appear in the targetlist in subLinkId order. This should
907-
* be safe enough given the way that the parser builds the targetlist
908-
* today. I wouldn't want to rely on it going forward, but since this
909-
* code has a limited lifespan it should be fine. We can partially
910-
* protect against problems with assertions below.
912+
* Append the new replacement-Params list to root->multiexpr_params.
913+
* Then its index in that list becomes the new subLinkId of the
914+
* SubPlan.
911915
*/
912-
new_multiexpr_params = lappend(new_multiexpr_params, params);
916+
root->multiexpr_params = lappend(root->multiexpr_params, params);
917+
oldId = splan->subLinkId;
918+
newId = list_length(root->multiexpr_params);
919+
Assert(newId > oldId);
920+
splan->subLinkId = newId;
921+
922+
/*
923+
* Add a mapping entry to param_mapping so that we can update the
924+
* associated Params below. Leave zeroes in the list for any
925+
* subLinkIds we don't encounter; those must have been converted to
926+
* initplans.
927+
*/
928+
while (list_length(param_mapping) < oldId)
929+
param_mapping = lappend_int(param_mapping, 0);
930+
lc2 = list_nth_cell(param_mapping, oldId - 1);
931+
lfirst_int(lc2) = newId;
913932
}
914933

915934
/*
916-
* Now we must find the Param nodes that reference the MULTIEXPR outputs
917-
* and update their sublink IDs so they'll reference the new outputs.
918-
* Fortunately, those too must be in the cloned targetlist, but they could
919-
* be buried under FieldStores and ArrayRefs and CoerceToDomains
920-
* (cf processIndirection()), and underneath those there could be an
921-
* implicit type coercion.
935+
* Unless all the MULTIEXPRs were converted to initplans, we must now find
936+
* the Param nodes that reference the MULTIEXPR outputs and update their
937+
* sublink IDs so they'll reference the new outputs. While such Params
938+
* must be in the cloned targetlist, they could be buried under stuff such
939+
* as FieldStores and ArrayRefs and type coercions.
922940
*/
923-
offset = list_length(root->multiexpr_params);
941+
if (param_mapping != NIL)
942+
SS_make_multiexprs_unique_walker((Node *) subroot->parse->targetList,
943+
(void *) param_mapping);
944+
}
924945

925-
foreach(lc, subroot->parse->targetList)
946+
/*
947+
* Locate PARAM_MULTIEXPR Params in an expression tree, and update as needed.
948+
* (We can update-in-place because the tree was already copied.)
949+
*/
950+
static bool
951+
SS_make_multiexprs_unique_walker(Node *node, void *context)
952+
{
953+
if (node == NULL)
954+
return false;
955+
if (IsA(node, Param))
926956
{
927-
TargetEntry *tent = (TargetEntry *) lfirst(lc);
928-
Node *expr;
929-
Param *p;
957+
Param *p = (Param *) node;
958+
List *param_mapping = (List *) context;
930959
int subqueryid;
931960
int colno;
961+
int newId;
932962

933-
expr = (Node *) tent->expr;
934-
while (expr)
935-
{
936-
if (IsA(expr, FieldStore))
937-
{
938-
FieldStore *fstore = (FieldStore *) expr;
939-
940-
expr = (Node *) linitial(fstore->newvals);
941-
}
942-
else if (IsA(expr, ArrayRef))
943-
{
944-
ArrayRef *aref = (ArrayRef *) expr;
945-
946-
if (aref->refassgnexpr == NULL)
947-
break;
948-
949-
expr = (Node *) aref->refassgnexpr;
950-
}
951-
else if (IsA(expr, CoerceToDomain))
952-
{
953-
CoerceToDomain *cdomain = (CoerceToDomain *) expr;
954-
955-
if (cdomain->coercionformat != COERCE_IMPLICIT_CAST)
956-
break;
957-
expr = (Node *) cdomain->arg;
958-
}
959-
else
960-
break;
961-
}
962-
expr = strip_implicit_coercions(expr);
963-
if (expr == NULL || !IsA(expr, Param))
964-
continue;
965-
p = (Param *) expr;
966963
if (p->paramkind != PARAM_MULTIEXPR)
967-
continue;
964+
return false;
968965
subqueryid = p->paramid >> 16;
969966
colno = p->paramid & 0xFFFF;
970-
Assert(subqueryid > 0 &&
971-
subqueryid <= list_length(new_multiexpr_params));
972-
Assert(colno > 0 &&
973-
colno <= list_length((List *) list_nth(new_multiexpr_params,
974-
subqueryid - 1)));
975-
subqueryid += offset;
976-
p->paramid = (subqueryid << 16) + colno;
977-
}
978967

979-
/* Finally, attach new replacement lists to the global list */
980-
root->multiexpr_params = list_concat(root->multiexpr_params,
981-
new_multiexpr_params);
968+
/*
969+
* If subqueryid doesn't have a mapping entry, it must refer to an
970+
* initplan, so don't change the Param.
971+
*/
972+
Assert(subqueryid > 0);
973+
if (subqueryid > list_length(param_mapping))
974+
return false;
975+
newId = list_nth_int(param_mapping, subqueryid - 1);
976+
if (newId == 0)
977+
return false;
978+
p->paramid = (newId << 16) + colno;
979+
return false;
980+
}
981+
return expression_tree_walker(node, SS_make_multiexprs_unique_walker,
982+
context);
982983
}
983984

984985

@@ -1061,6 +1062,7 @@ SS_process_ctes(PlannerInfo *root)
10611062
*/
10621063
splan = makeNode(SubPlan);
10631064
splan->subLinkType = CTE_SUBLINK;
1065+
splan->subLinkId = 0;
10641066
splan->testexpr = NULL;
10651067
splan->paramIds = NIL;
10661068
get_first_col_type(plan, &splan->firstColType, &splan->firstColTypmod,
@@ -2858,6 +2860,7 @@ SS_make_initplan_from_plan(PlannerInfo *root,
28582860
*/
28592861
node = makeNode(SubPlan);
28602862
node->subLinkType = EXPR_SUBLINK;
2863+
node->subLinkId = 0;
28612864
node->plan_id = list_length(root->glob->subplans);
28622865
node->plan_name = psprintf("InitPlan %d (returns $%d)",
28632866
node->plan_id, prm->paramid);

‎src/include/nodes/primnodes.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -714,6 +714,8 @@ typedef struct SubPlan
714714
/* Estimated execution costs: */
715715
Cost startup_cost; /* one-time setup cost */
716716
Cost per_call_cost; /* cost for each subplan evaluation */
717+
/* Copied from original SubLink, but placed at end for ABI stability */
718+
int subLinkId; /* ID (1..n); 0 if not MULTIEXPR */
717719
} SubPlan;
718720

719721
/*

‎src/test/regress/expected/inherit.out

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1748,23 +1748,36 @@ reset enable_bitmapscan;
17481748
--
17491749
-- Check handling of MULTIEXPR SubPlans in inherited updates
17501750
--
1751-
create table inhpar(f1 int, f2 text[]);
1751+
create table inhpar(f1 int, f2 text[], f3 int);
17521752
insert into inhpar select generate_series(1,10);
17531753
create table inhcld() inherits(inhpar);
17541754
insert into inhcld select generate_series(11,10000);
17551755
vacuum analyze inhcld;
17561756
vacuum analyze inhpar;
17571757
explain (verbose, costs off)
1758-
update inhpar set (f1, f2[1]) = (select p2.unique2, p2.stringu1
1759-
from int4_tbl limit 1)
1758+
update inhpar set
1759+
(f1, f2[1]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
1760+
(f2[2], f2[3]) = (select 'x', 'y' from int4_tbl limit 1),
1761+
(f3, f2[4]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
1762+
(f2[5], f2[6]) = (select 'x', 'y' from int4_tbl limit 1)
17601763
from onek p2 where inhpar.f1 = p2.unique1;
1761-
QUERY PLAN
1762-
-------------------------------------------------------------------------------------------
1764+
QUERY PLAN
1765+
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
17631766
Update on public.inhpar
17641767
Update on public.inhpar
17651768
Update on public.inhcld
1769+
InitPlan 2 (returns $4,$5)
1770+
-> Limit
1771+
Output: 'x'::text, 'y'::text
1772+
-> Seq Scan on public.int4_tbl int4_tbl_1
1773+
Output: 'x'::text, 'y'::text
1774+
InitPlan 4 (returns $10,$11)
1775+
-> Limit
1776+
Output: 'x'::text, 'y'::text
1777+
-> Seq Scan on public.int4_tbl int4_tbl_3
1778+
Output: 'x'::text, 'y'::text
17661779
-> Merge Join
1767-
Output: $4, inhpar.f2[1] := $5, (SubPlan 1 (returns $2,$3)), inhpar.ctid, p2.ctid
1780+
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
17681781
Merge Cond: (p2.unique1 = inhpar.f1)
17691782
-> Index Scan using onek_unique1 on public.onek p2
17701783
Output: p2.unique2, p2.stringu1, p2.ctid, p2.unique1
@@ -1778,19 +1791,27 @@ from onek p2 where inhpar.f1 = p2.unique1;
17781791
Output: (p2.unique2), (p2.stringu1)
17791792
-> Seq Scan on public.int4_tbl
17801793
Output: p2.unique2, p2.stringu1
1794+
SubPlan 3 (returns $8,$9)
1795+
-> Limit
1796+
Output: (p2.unique2), (p2.stringu1)
1797+
-> Seq Scan on public.int4_tbl int4_tbl_2
1798+
Output: p2.unique2, p2.stringu1
17811799
-> Hash Join
1782-
Output: $6, inhcld.f2[1] := $7, (SubPlan 1 (returns $2,$3)), inhcld.ctid, p2.ctid
1800+
Output: $16, (((((inhcld.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, inhcld.ctid, p2.ctid
17831801
Hash Cond: (inhcld.f1 = p2.unique1)
17841802
-> Seq Scan on public.inhcld
17851803
Output: inhcld.f2, inhcld.ctid, inhcld.f1
17861804
-> Hash
17871805
Output: p2.unique2, p2.stringu1, p2.ctid, p2.unique1
17881806
-> Seq Scan on public.onek p2
17891807
Output: p2.unique2, p2.stringu1, p2.ctid, p2.unique1
1790-
(27 rows)
1808+
(42 rows)
17911809

1792-
update inhpar set (f1, f2[1]) = (select p2.unique2, p2.stringu1
1793-
from int4_tbl limit 1)
1810+
update inhpar set
1811+
(f1, f2[1]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
1812+
(f2[2], f2[3]) = (select 'x', 'y' from int4_tbl limit 1),
1813+
(f3, f2[4]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
1814+
(f2[5], f2[6]) = (select 'x', 'y' from int4_tbl limit 1)
17941815
from onek p2 where inhpar.f1 = p2.unique1;
17951816
drop table inhpar cascade;
17961817
NOTICE: drop cascades to table inhcld

‎src/test/regress/sql/inherit.sql

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -653,19 +653,25 @@ reset enable_bitmapscan;
653653
--
654654
-- Check handling of MULTIEXPR SubPlans in inherited updates
655655
--
656-
create table inhpar(f1 int, f2 text[]);
656+
create table inhpar(f1 int, f2 text[], f3 int);
657657
insert into inhpar select generate_series(1,10);
658658
create table inhcld() inherits(inhpar);
659659
insert into inhcld select generate_series(11,10000);
660660
vacuum analyze inhcld;
661661
vacuum analyze inhpar;
662662

663663
explain (verbose, costs off)
664-
update inhpar set (f1, f2[1]) = (select p2.unique2, p2.stringu1
665-
from int4_tbl limit 1)
664+
update inhpar set
665+
(f1, f2[1]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
666+
(f2[2], f2[3]) = (select 'x', 'y' from int4_tbl limit 1),
667+
(f3, f2[4]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
668+
(f2[5], f2[6]) = (select 'x', 'y' from int4_tbl limit 1)
666669
from onek p2 where inhpar.f1 = p2.unique1;
667-
update inhpar set (f1, f2[1]) = (select p2.unique2, p2.stringu1
668-
from int4_tbl limit 1)
670+
update inhpar set
671+
(f1, f2[1]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
672+
(f2[2], f2[3]) = (select 'x', 'y' from int4_tbl limit 1),
673+
(f3, f2[4]) = (select p2.unique2, p2.stringu1 from int4_tbl limit 1),
674+
(f2[5], f2[6]) = (select 'x', 'y' from int4_tbl limit 1)
669675
from onek p2 where inhpar.f1 = p2.unique1;
670676

671677
drop table inhpar cascade;

0 commit comments

Comments
 (0)
0