Skip to content

Commit 9eaba06

Browse files
tglsfdctglsfdc
committed
Fix MULTIEXPR_SUBLINK with partitioned target tables, yet again.
We already tried to fix this in commits 3f7323c et al (and follow-on fixes), but now it emerges that there are still unfixed cases; moreover, these cases affect all branches not only pre-v14. I thought we had eliminated all cases of making multiple clones of an UPDATE's target list when we nuked inheritance_planner. But it turns out we still do that in some partitioned-UPDATE cases, notably including INSERT ... ON CONFLICT UPDATE, because ExecInitPartitionInfo thinks it's okay to clone and modify the parent's targetlist. This fix is based on a suggestion from Andres Freund: let's stop abusing the ParamExecData.execPlan mechanism, which was only ever meant to handle initplans, and instead solve the execution timing problem by having the expression compiler move MULTIEXPR_SUBLINK steps to the front of their expression step lists. This is feasible because (a) all branches still in support compile the entire targetlist of an UPDATE into a single ExprState, and (b) we know that all MULTIEXPR_SUBLINKs do need to be evaluated --- none could be buried inside a CASE, for example. There is a minor semantics change concerning the order of execution of the MULTIEXPR's subquery versus other parts of the parent targetlist, but that seems like something we can get away with. By doing that, we no longer need to worry about whether different clones of a MULTIEXPR_SUBLINK share output Params; their usage of that data structure won't overlap. Per bug #17800 from Alexander Lakhin. Back-patch to all supported branches. In v13 and earlier, we can revert 3f7323c and follow-on fixes; however, I chose to keep the SubPlan.subLinkId field added in ccbb54c. We don't need that anymore in the core code, but it's cheap enough to fill, and removing a plan node field in a minor release seems like it'd be asking for trouble. Andres Freund and Tom Lane Discussion: https://postgr.es/m/17800-ff90866b3906c964@postgresql.org
1 parent 27ff93d commit 9eaba06

File tree

5 files changed

+332
-119
lines changed

5 files changed

+332
-119
lines changed

‎src/backend/executor/execExpr.c

Lines changed: 112 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -51,22 +51,25 @@
5151
#include "utils/typcache.h"
5252

5353

54-
typedef struct LastAttnumInfo
54+
typedef struct ExprSetupInfo
5555
{
56+
/* Highest attribute numbers fetched from inner/outer/scan tuple slots: */
5657
AttrNumber last_inner;
5758
AttrNumber last_outer;
5859
AttrNumber last_scan;
59-
} LastAttnumInfo;
60+
/* MULTIEXPR SubPlan nodes appearing in the expression: */
61+
List *multiexpr_subplans;
62+
} ExprSetupInfo;
6063

6164
static void ExecReadyExpr(ExprState *state);
6265
static void ExecInitExprRec(Expr *node, ExprState *state,
6366
Datum *resv, bool *resnull);
6467
static void ExecInitFunc(ExprEvalStep *scratch, Expr *node, List *args,
6568
Oid funcid, Oid inputcollid,
6669
ExprState *state);
67-
static void ExecInitExprSlots(ExprState *state, Node *node);
68-
static void ExecPushExprSlots(ExprState *state, LastAttnumInfo *info);
69-
static bool get_last_attnums_walker(Node *node, LastAttnumInfo *info);
70+
static void ExecCreateExprSetupSteps(ExprState *state, Node *node);
71+
static void ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info);
72+
static bool expr_setup_walker(Node *node, ExprSetupInfo *info);
7073
static bool ExecComputeSlotInfo(ExprState *state, ExprEvalStep *op);
7174
static void ExecInitWholeRowVar(ExprEvalStep *scratch, Var *variable,
7275
ExprState *state);
@@ -135,8 +138,8 @@ ExecInitExpr(Expr *node, PlanState *parent)
135138
state->parent = parent;
136139
state->ext_params = NULL;
137140

138-
/* Insert EEOP_*_FETCHSOME steps as needed */
139-
ExecInitExprSlots(state, (Node *) node);
141+
/* Insert setup steps as needed */
142+
ExecCreateExprSetupSteps(state, (Node *) node);
140143

141144
/* Compile the expression proper */
142145
ExecInitExprRec(node, state, &state->resvalue, &state->resnull);
@@ -172,8 +175,8 @@ ExecInitExprWithParams(Expr *node, ParamListInfo ext_params)
172175
state->parent = NULL;
173176
state->ext_params = ext_params;
174177

175-
/* Insert EEOP_*_FETCHSOME steps as needed */
176-
ExecInitExprSlots(state, (Node *) node);
178+
/* Insert setup steps as needed */
179+
ExecCreateExprSetupSteps(state, (Node *) node);
177180

178181
/* Compile the expression proper */
179182
ExecInitExprRec(node, state, &state->resvalue, &state->resnull);
@@ -227,8 +230,8 @@ ExecInitQual(List *qual, PlanState *parent)
227230
/* mark expression as to be used with ExecQual() */
228231
state->flags = EEO_FLAG_IS_QUAL;
229232

230-
/* Insert EEOP_*_FETCHSOME steps as needed */
231-
ExecInitExprSlots(state, (Node *) qual);
233+
/* Insert setup steps as needed */
234+
ExecCreateExprSetupSteps(state, (Node *) qual);
232235

233236
/*
234237
* ExecQual() needs to return false for an expression returning NULL. That
@@ -371,8 +374,8 @@ ExecBuildProjectionInfo(List *targetList,
371374

372375
state->resultslot = slot;
373376

374-
/* Insert EEOP_*_FETCHSOME steps as needed */
375-
ExecInitExprSlots(state, (Node *) targetList);
377+
/* Insert setup steps as needed */
378+
ExecCreateExprSetupSteps(state, (Node *) targetList);
376379

377380
/* Now compile each tlist column */
378381
foreach(lc, targetList)
@@ -523,7 +526,7 @@ ExecBuildUpdateProjection(List *targetList,
523526
int nAssignableCols;
524527
bool sawJunk;
525528
Bitmapset *assignedCols;
526-
LastAttnumInfo deform = {0, 0, 0};
529+
ExprSetupInfo deform = {0, 0, 0, NIL};
527530
ExprEvalStep scratch = {0};
528531
int outerattnum;
529532
ListCell *lc,
@@ -602,17 +605,18 @@ ExecBuildUpdateProjection(List *targetList,
602605
* number of columns of the "outer" tuple.
603606
*/
604607
if (evalTargetList)
605-
get_last_attnums_walker((Node *) targetList, &deform);
608+
expr_setup_walker((Node *) targetList, &deform);
606609
else
607610
deform.last_outer = nAssignableCols;
608611

609-
ExecPushExprSlots(state, &deform);
612+
ExecPushExprSetupSteps(state, &deform);
610613

611614
/*
612615
* Now generate code to evaluate the tlist's assignable expressions or
613616
* fetch them from the outer tuple, incidentally validating that they'll
614617
* be of the right data type. The checks above ensure that the forboth()
615-
* will iterate over exactly the non-junk columns.
618+
* will iterate over exactly the non-junk columns. Note that we don't
619+
* bother evaluating any remaining resjunk columns.
616620
*/
617621
outerattnum = 0;
618622
forboth(lc, targetList, lc2, targetColnos)
@@ -675,22 +679,6 @@ ExecBuildUpdateProjection(List *targetList,
675679
outerattnum++;
676680
}
677681

678-
/*
679-
* If we're evaluating the tlist, must evaluate any resjunk columns too.
680-
* (This matters for things like MULTIEXPR_SUBLINK SubPlans.)
681-
*/
682-
if (evalTargetList)
683-
{
684-
for_each_cell(lc, targetList, lc)
685-
{
686-
TargetEntry *tle = lfirst_node(TargetEntry, lc);
687-
688-
Assert(tle->resjunk);
689-
ExecInitExprRec(tle->expr, state,
690-
&state->resvalue, &state->resnull);
691-
}
692-
}
693-
694682
/*
695683
* Now generate code to copy over any old columns that were not assigned
696684
* to, and to ensure that dropped columns are set to NULL.
@@ -1402,6 +1390,21 @@ ExecInitExprRec(Expr *node, ExprState *state,
14021390
SubPlan *subplan = (SubPlan *) node;
14031391
SubPlanState *sstate;
14041392

1393+
/*
1394+
* Real execution of a MULTIEXPR SubPlan has already been
1395+
* done. What we have to do here is return a dummy NULL record
1396+
* value in case this targetlist element is assigned
1397+
* someplace.
1398+
*/
1399+
if (subplan->subLinkType == MULTIEXPR_SUBLINK)
1400+
{
1401+
scratch.opcode = EEOP_CONST;
1402+
scratch.d.constval.value = (Datum) 0;
1403+
scratch.d.constval.isnull = true;
1404+
ExprEvalPushStep(state, &scratch);
1405+
break;
1406+
}
1407+
14051408
if (!state->parent)
14061409
elog(ERROR, "SubPlan found with no parent plan");
14071410

@@ -2553,36 +2556,38 @@ ExecInitFunc(ExprEvalStep *scratch, Expr *node, List *args, Oid funcid,
25532556
}
25542557

25552558
/*
2556-
* Add expression steps deforming the ExprState's inner/outer/scan slots
2557-
* as much as required by the expression.
2559+
* Add expression steps performing setup that's needed before any of the
2560+
* main execution of the expression.
25582561
*/
25592562
static void
2560-
ExecInitExprSlots(ExprState *state, Node *node)
2563+
ExecCreateExprSetupSteps(ExprState *state, Node *node)
25612564
{
2562-
LastAttnumInfo info = {0, 0, 0};
2565+
ExprSetupInfo info = {0, 0, 0, NIL};
25632566

2564-
/*
2565-
* Figure out which attributes we're going to need.
2566-
*/
2567-
get_last_attnums_walker(node, &info);
2567+
/* Prescan to find out what we need. */
2568+
expr_setup_walker(node, &info);
25682569

2569-
ExecPushExprSlots(state, &info);
2570+
/* And generate those steps. */
2571+
ExecPushExprSetupSteps(state, &info);
25702572
}
25712573

25722574
/*
2573-
* Add steps deforming the ExprState's inner/out/scan slots as much as
2574-
* indicated by info. This is useful when building an ExprState covering more
2575-
* than one expression.
2575+
* Add steps performing expression setup as indicated by "info".
2576+
* This is useful when building an ExprState covering more than one expression.
25762577
*/
25772578
static void
2578-
ExecPushExprSlots(ExprState *state, LastAttnumInfo *info)
2579+
ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info)
25792580
{
25802581
ExprEvalStep scratch = {0};
2582+
ListCell *lc;
25812583

25822584
scratch.resvalue = NULL;
25832585
scratch.resnull = NULL;
25842586

2585-
/* Emit steps as needed */
2587+
/*
2588+
* Add steps deforming the ExprState's inner/outer/scan slots as much as
2589+
* required by any Vars appearing in the expression.
2590+
*/
25862591
if (info->last_inner > 0)
25872592
{
25882593
scratch.opcode = EEOP_INNER_FETCHSOME;
@@ -2613,13 +2618,48 @@ ExecPushExprSlots(ExprState *state, LastAttnumInfo *info)
26132618
if (ExecComputeSlotInfo(state, &scratch))
26142619
ExprEvalPushStep(state, &scratch);
26152620
}
2621+
2622+
/*
2623+
* Add steps to execute any MULTIEXPR SubPlans appearing in the
2624+
* expression. We need to evaluate these before any of the Params
2625+
* referencing their outputs are used, but after we've prepared for any
2626+
* Var references they may contain. (There cannot be cross-references
2627+
* between MULTIEXPR SubPlans, so we needn't worry about their order.)
2628+
*/
2629+
foreach(lc, info->multiexpr_subplans)
2630+
{
2631+
SubPlan *subplan = (SubPlan *) lfirst(lc);
2632+
SubPlanState *sstate;
2633+
2634+
Assert(subplan->subLinkType == MULTIEXPR_SUBLINK);
2635+
2636+
/* This should match what ExecInitExprRec does for other SubPlans: */
2637+
2638+
if (!state->parent)
2639+
elog(ERROR, "SubPlan found with no parent plan");
2640+
2641+
sstate = ExecInitSubPlan(subplan, state->parent);
2642+
2643+
/* add SubPlanState nodes to state->parent->subPlan */
2644+
state->parent->subPlan = lappend(state->parent->subPlan,
2645+
sstate);
2646+
2647+
scratch.opcode = EEOP_SUBPLAN;
2648+
scratch.d.subplan.sstate = sstate;
2649+
2650+
/* The result can be ignored, but we better put it somewhere */
2651+
scratch.resvalue = &state->resvalue;
2652+
scratch.resnull = &state->resnull;
2653+
2654+
ExprEvalPushStep(state, &scratch);
2655+
}
26162656
}
26172657

26182658
/*
2619-
* get_last_attnums_walker: expression walker for ExecInitExprSlots
2659+
* expr_setup_walker: expression walker for ExecCreateExprSetupSteps
26202660
*/
26212661
static bool
2622-
get_last_attnums_walker(Node *node, LastAttnumInfo *info)
2662+
expr_setup_walker(Node *node, ExprSetupInfo *info)
26232663
{
26242664
if (node == NULL)
26252665
return false;
@@ -2647,6 +2687,16 @@ get_last_attnums_walker(Node *node, LastAttnumInfo *info)
26472687
return false;
26482688
}
26492689

2690+
/* Collect all MULTIEXPR SubPlans, too */
2691+
if (IsA(node, SubPlan))
2692+
{
2693+
SubPlan *subplan = (SubPlan *) node;
2694+
2695+
if (subplan->subLinkType == MULTIEXPR_SUBLINK)
2696+
info->multiexpr_subplans = lappend(info->multiexpr_subplans,
2697+
subplan);
2698+
}
2699+
26502700
/*
26512701
* Don't examine the arguments or filters of Aggrefs or WindowFuncs,
26522702
* because those do not represent expressions to be evaluated within the
@@ -2659,7 +2709,7 @@ get_last_attnums_walker(Node *node, LastAttnumInfo *info)
26592709
return false;
26602710
if (IsA(node, GroupingFunc))
26612711
return false;
2662-
return expression_tree_walker(node, get_last_attnums_walker,
2712+
return expression_tree_walker(node, expr_setup_walker,
26632713
(void *) info);
26642714
}
26652715

@@ -3278,7 +3328,7 @@ ExecBuildAggTrans(AggState *aggstate, AggStatePerPhase phase,
32783328
PlanState *parent = &aggstate->ss.ps;
32793329
ExprEvalStep scratch = {0};
32803330
bool isCombine = DO_AGGSPLIT_COMBINE(aggstate->aggsplit);
3281-
LastAttnumInfo deform = {0, 0, 0};
3331+
ExprSetupInfo deform = {0, 0, 0, NIL};
32823332

32833333
state->expr = (Expr *) aggstate;
32843334
state->parent = parent;
@@ -3294,18 +3344,18 @@ ExecBuildAggTrans(AggState *aggstate, AggStatePerPhase phase,
32943344
{
32953345
AggStatePerTrans pertrans = &aggstate->pertrans[transno];
32963346

3297-
get_last_attnums_walker((Node *) pertrans->aggref->aggdirectargs,
3298-
&deform);
3299-
get_last_attnums_walker((Node *) pertrans->aggref->args,
3300-
&deform);
3301-
get_last_attnums_walker((Node *) pertrans->aggref->aggorder,
3302-
&deform);
3303-
get_last_attnums_walker((Node *) pertrans->aggref->aggdistinct,
3304-
&deform);
3305-
get_last_attnums_walker((Node *) pertrans->aggref->aggfilter,
3306-
&deform);
3347+
expr_setup_walker((Node *) pertrans->aggref->aggdirectargs,
3348+
&deform);
3349+
expr_setup_walker((Node *) pertrans->aggref->args,
3350+
&deform);
3351+
expr_setup_walker((Node *) pertrans->aggref->aggorder,
3352+
&deform);
3353+
expr_setup_walker((Node *) pertrans->aggref->aggdistinct,
3354+
&deform);
3355+
expr_setup_walker((Node *) pertrans->aggref->aggfilter,
3356+
&deform);
33073357
}
3308-
ExecPushExprSlots(state, &deform);
3358+
ExecPushExprSetupSteps(state, &deform);
33093359

33103360
/*
33113361
* Emit instructions for each transition value / grouping set combination.

0 commit comments

Comments
 (0)