8000 Repair rare failure of MULTIEXPR_SUBLINK subplans in inherited updates. · postgres/postgres@d9ebc58 · GitHub
[go: up one dir, main page]
Skip to content

Commit d9ebc58

Browse files
tglsfdctglsfdc
committed
Repair rare failure of MULTIEXPR_SUBLINK subplans in inherited updates.
Prior to v14, if we have a MULTIEXPR SubPlan (that is, use of the syntax UPDATE ... SET (c1, ...) = (SELECT ...)) in an UPDATE with an inherited or partitioned target table, inheritance_planner() will clone the targetlist and therefore also the MULTIEXPR SubPlan and the Param nodes referencing it for each child target table. Up to now, we've allowed all the clones to share the underlying subplan as well as the output parameter IDs -- that is, the runtime ParamExecData slots. That technique is borrowed from the far older code that supports initplans, and it works okay in that case because the cloned SubPlan nodes are essentially identical. So it doesn't matter which one of the clones the shared ParamExecData.execPlan field might point to. However, this fails to hold for MULTIEXPR SubPlans, because they can have nonempty "args" lists (values to be passed into the subplan), and those lists could get mutated to different states in the various clones. In the submitted reproducer, as well as the test case added here, one clone contains Vars with varno OUTER_VAR where another has INNER_VAR, because the child tables are respectively on the outer or inner side of the join. Sharing the execPlan pointer can result in trying to evaluate an args list that doesn't match the local execution state, with mayhem ensuing. The result often is to trigger consistency checks in the executor, but I believe this could end in a crash or incorrect updates. To fix, assign new Param IDs to each of the cloned SubPlans, so that they don't share ParamExecData slots at runtime. It still seems fine for the clones to share the underlying subplan, and extra ParamExecData slots are cheap enough that this fix shouldn't cost much. This has been busted since we invented MULTIEXPR SubPlans in 9.5. Probably the lack of previous reports is because query plans in which the different clones of a MULTIEXPR mutate to effectively-different states are pretty rare. There's no issue in v14 and later, because without inheritance_planner() there's never a reason to clone MULTIEXPR SubPlans. Per bug #17596 from Andre Lin. Patch v10-v13 only. Discussion: https://postgr.es/m/17596-c5357f61427a81dc@postgresql.org
1 parent 1d40200 commit d9ebc58

File tree

6 files changed

+184
-0
lines changed

6 files changed

+184
-0
lines changed

‎src/backend/executor/nodeSubplan.c

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,21 @@ ExecScanSubPlan(SubPlanState *node,
246246
* ones, so this should be safe.) Unlike ExecReScanSetParamPlan, we do
247247
* *not* set bits in the parent plan node's chgParam, because we don't
248248
* want to cause a rescan of the parent.
249+
*
250+
* Note: we are also relying on MULTIEXPR SubPlans not sharing any output
251+
* parameters with other SubPlans, because if one does then it is unclear
252+
* which SubPlanState node the parameter's execPlan field will be pointing
253+
* to when we come to evaluate the parameter. We can allow plain initplan
254+
* SubPlans to share output parameters, because it doesn't actually matter
255+
* which initplan SubPlan we reference as long as they all point to the
256+
* same underlying subplan. However, that fails to hold for MULTIEXPRs
257+
* because they can have non-empty args lists, and the "same" args might
258+
* have mutated into different forms in different parts of a plan tree.
259+
* There is not a problem in ordinary queries because MULTIEXPR will
260+
* appear only in an UPDATE's top-level target list, so it won't get
261+
* duplicated anyplace. However, when inheritance_planner clones a
262+
* partially-planned targetlist it must take care to assign non-duplicate
263+
* param IDs to the cloned copy.
249264
*/
250265
if (subLinkType == MULTIEXPR_SUBLINK)
251266
{

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1480,6 +1480,10 @@ inheritance_planner(PlannerInfo *root)
14801480
/* and we haven't created PlaceHolderInfos, either */
14811481
Assert(subroot->placeholder_list == NIL);
14821482

1483+
/* Fix MULTIEXPR_SUBLINK params if any */
1484+
if (root->multiexpr_params)
1485+
SS_make_multiexprs_unique(root, subroot);
1486+
14831487
/* Generate Path(s) for accessing this result relation */
14841488
grouping_planner(subroot, true, 0.0 /* retrieve all tuples */ );
14851489

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

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -852,6 +852,101 @@ hash_ok_operator(OpExpr *expr)
852852
}
853853
}
854854

855+
/*
856+
* SS_make_multiexprs_unique
857+
*
858+
* After cloning an UPDATE targetlist that contains MULTIEXPR_SUBLINK
859+
* SubPlans, inheritance_planner() must call this to assign new, unique Param
860+
* IDs to the cloned MULTIEXPR_SUBLINKs' output parameters. See notes in
861+
* ExecScanSubPlan.
862+
*/
863+
void
864+
SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot)
865+
{
866+
List *new_multiexpr_params = NIL;
867+
int offset;
868+
ListCell *lc;
869+
870+
/*
871+
* Find MULTIEXPR SubPlans in the cloned query. We need only look at the
872+
* top level of the targetlist.
873+
*/
874+
foreach(lc, subroot->parse->targetList)
875+
{
876+
TargetEntry *tent = (TargetEntry *) lfirst(lc);
877+
SubPlan *splan;
878+
Plan *plan;
879+
List *params;
880+
881+
if (!IsA(tent->expr, SubPlan))
882+
continue;
883+
splan = (SubPlan *) tent->expr;
884+
if (splan->subLinkType != MULTIEXPR_SUBLINK)
885+
continue;
886+
887+
/* Found one, get the associated subplan */
888+
plan = (Plan *) list_nth(root->glob->subplans, splan->plan_id - 1);
889+
890+
/*
891+
* Generate new PARAM_EXEC Param nodes, and overwrite splan->setParam
892+
* with their IDs. This is just like what build_subplan did when it
893+
* made the SubPlan node we're cloning. But because the param IDs are
894+
* assigned globally, we'll get new IDs. (We assume here that the
895+
* subroot's tlist is a clone we can scribble on.)
896+
*/
897+
params = generate_subquery_params(root,
898+
plan->targetlist,
899+
&splan->setParam);
900+
901+
/*
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.
911+
*/
912+
new_multiexpr_params = lappend(new_multiexpr_params, params);
913+
}
914+
915+
/*
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 at top level of the cloned targetlist.
919+
*/
920+
offset = list_length(root->multiexpr_params);
921+
922+
foreach(lc, subroot->parse->targetList)
923+
{
924+
TargetEntry *tent = (TargetEntry *) lfirst(lc);
925+
Param *p;
926+
int subqueryid;
927+
int colno;
928+
929+
if (!IsA(tent->expr, Param))
930+
continue;
931+
p = (Param *) tent->expr;
932+
if (p->paramkind != PARAM_MULTIEXPR)
933+
continue;
934+
subqueryid = p->paramid >> 16;
935+
colno = p->paramid & 0xFFFF;
936+
Assert(subqueryid > 0 &&
937+
subqueryid <= list_length(new_multiexpr_params));
938+
Assert(colno > 0 &&
939+
colno <= list_length((List *) list_nth(new_multiexpr_params,
940+
subqueryid - 1)));
941+
subqueryid += offset;
942+
p->paramid = (subqueryid << 16) + colno;
943+
}
944+
945+
/* Finally, attach new replacement lists to the global list */
946+
root->multiexpr_params = list_concat(root->multiexpr_params,
947+
new_multiexpr_params);
948+
}
949+
855950

856951
/*
857952
* SS_process_ctes: process a query's WITH list

‎src/include/optimizer/subselect.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
#include "nodes/plannodes.h"
1717
#include "nodes/relation.h"
1818

19+
extern void SS_make_multiexprs_unique(PlannerInfo *root, PlannerInfo *subroot);
1920
extern void SS_process_ctes(PlannerInfo *root);
2021
extern JoinExpr *convert_ANY_sublink_to_join(PlannerInfo *root,
2122
SubLink *sublink,

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1746,6 +1746,55 @@ reset enable_seqscan;
17461746
reset enable_indexscan;
17471747
reset enable_bitmapscan;
17481748
--
1749+
-- Check handling of MULTIEXPR SubPlans in inherited updates
1750+
--
1751+
create table inhpar(f1 int, f2 name);
1752+
insert into inhpar select generate_series(1,10);
1753+
create table inhcld() inherits(inhpar);
1754+
insert into inhcld select generate_series(11,10000);
1755+
vacuum analyze inhcld;
1756+
vacuum analyze inhpar;
1757+
explain (verbose, costs off)
1758+
update inhpar set (f1, f2) = (select p2.unique2, p2.stringu1
1759+
from int4_tbl limit 1)
1760+
from onek p2 where inhpar.f1 = p2.unique1;
1761+
QUERY PLAN
1762+
---------------------------------------------------------------------------
1763+
Update on public.inhpar
1764+
Update on public.inhpar
1765+
Update on public.inhcld
1766+
-> Merge Join
1767+
Output: $4, $5, (SubPlan 1 (returns $2,$3)), inhpar.ctid, p2.ctid
1768+
Merge Cond: (p2.unique1 = inhpar.f1)
1769+
-> Index Scan using onek_unique1 on public.onek p2
1770+
Output: p2.unique2, p2.stringu1, p2.ctid, p2.unique1
1771+
-> Sort
1772+
Output: inhpar.ctid, inhpar.f1
1773+
Sort Key: inhpar.f1
1774+
-> Seq Scan on public.inhpar
1775+
Output: inhpar.ctid, inhpar.f1
1776+
SubPlan 1 (returns $2,$3)
1777+
-> Limit
1778+
Output: (p2.unique2), (p2.stringu1)
1779+
-> Seq Scan on public.int4_tbl
1780+
Output: p2.unique2, p2.stringu1
1781+
-> Hash Join
1782+
Output: $6, $7, (SubPlan 1 (returns $2,$3)), inhcld.ctid, p2.ctid
1783+
Hash Cond: (inhcld.f1 = p2.unique1)
1784+
-> Seq Scan on public.inhcld
1785+
Output: inhcld.ctid, inhcld.f1
1786+
-> Hash
1787+
Output: p2.unique2, p2.stringu1, p2.ctid, p2.unique1
1788+
-> Seq Scan on public.onek p2
1789+
Output: p2.unique2, p2.stringu1, p2.ctid, p2.unique1
1790+
(27 rows)
1791+
1792+
update inhpar set (f1, f2) = (select p2.unique2, p2.stringu1
1793+
from int4_tbl limit 1)
1794+
from onek p2 where inhpar.f1 = p2.unique1;
1795+
drop table inhpar cascade;
1796+
NOTICE: drop cascades to table inhcld
1797+
--
17491798
-- Check handling of a constant-null CHECK constraint
17501799
--
17511800
create table cnullparent (f1 int);

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,26 @@ reset enable_seqscan;
650650
reset enable_indexscan;
651651
reset enable_bitmapscan;
652652

653+
--
654+
-- Check handling of MULTIEXPR SubPlans in inherited updates
655+
--
656+
create table inhpar(f1 int, f2 name);
657+
insert into inhpar select generate_series(1,10);
658+
create table inhcld() inherits(inhpar);
659+
insert into inhcld select generate_series(11,10000);
660+
vacuum analyze inhcld;
661+
vacuum analyze inhpar;
662+
663+
explain (verbose, costs off)
664+
update inhpar set (f1, f2) = (select p2.unique2, p2.stringu1
665+
from int4_tbl limit 1)
666+
from onek p2 where inhpar.f1 = p2.unique1;
667+
update inhpar set (f1, f2) = (select p2.unique2, p2.stringu1
668+
from int4_tbl limit 1)
669+
from onek p2 where inhpar.f1 = p2.unique1;
670+
671+
drop table inhpar cascade;
672+
653673
--
654674
-- Check handling of a constant-null CHECK constraint
655675
--

0 commit comments

Comments
 (0)
0