Skip to content

Commit 282778f

Browse files
[#25147] YSQL: Import 'Fix MULTIEXPR_SUBLINK with partitioned target tables, yet again.'
Summary: Import upstream postgres commit a033f9165c2c024756d9cd3033263724d53fd9ef from REL_15_STABLE. (master commit: 87f3667ec079c4acc2c87be51bf41e54d0b6f698) This is needed to fix a crash/error involving UPDATE projections on partitions with out-of-order columns. **Changes from upstream commit** - Import regress test validating fix from `inherit.*` to `yb.port.inherit.*`. - Remove GUC disabling INSERT ... ON CONFLICT read batching from specific queries in `yb.port.update.*`. - Add tests involving UPDATE projections on partitions with out-of-order columns to `yb.orig.partition_out_of_order.*`. **Upstream commit message** ``` We already tried to fix this in commits 3f7323cbb 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 3f7323cbb and follow-on fixes; however, I chose to keep the SubPlan.subLinkId field added in ccbb54c72. 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/[email protected] ``` Jira: DB-14307 Test Plan: Run the following tests: ``` ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressInherit' ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressPartitions#misc' ./yb_build.sh --java-test 'org.yb.pgsql.TestPgRegressPartitions#yb_partitions_tests' ``` Reviewers: jason Reviewed By: jason Subscribers: smishra, yql Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D42955
1 parent b0f8213 commit 282778f

12 files changed

+506
-124
lines changed

src/lint/upstream_repositories.csv

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
local_path,upstream_path,repository,commit
2-
src/postgres,,https://github.com/yugabyte/postgres,c06a3a2d14da337a781d6c135bf1911ff562b72d
2+
src/postgres,,https://github.com/yugabyte/postgres,a033f9165c2c024756d9cd3033263724d53fd9ef
33
src/postgres/contrib/passwordcheck,passwordcheck_extra,https://github.com/michaelpq/pg_plugins,66a75ea0ca0706bef7b1e17055aa5fd364189fec
44
src/postgres/third-party-extensions/hypopg,,https://github.com/HypoPG/hypopg,97026c0bff16b905c555acdd82e15979fa6b9781
55
src/postgres/third-party-extensions/orafce,,https://github.com/orafce/orafce,6adbd66c429dabb828f10d764d356824efeb8396

src/postgres/src/backend/executor/execExpr.c

+112-62
Original file line numberDiff line numberDiff line change
@@ -56,22 +56,25 @@
5656
#include "pg_yb_utils.h"
5757

5858

59-
typedef struct LastAttnumInfo
59+
typedef struct ExprSetupInfo
6060
{
61+
/* Highest attribute numbers fetched from inner/outer/scan tuple slots: */
6162
AttrNumber last_inner;
6263
AttrNumber last_outer;
6364
AttrNumber last_scan;
64-
} LastAttnumInfo;
65+
/* MULTIEXPR SubPlan nodes appearing in the expression: */
66+
List *multiexpr_subplans;
67+
} ExprSetupInfo;
6568

6669
static void ExecReadyExpr(ExprState *state);
6770
static void ExecInitExprRec(Expr *node, ExprState *state,
6871
Datum *resv, bool *resnull);
6972
static void ExecInitFunc(ExprEvalStep *scratch, Expr *node, List *args,
7073
Oid funcid, Oid inputcollid,
7174
ExprState *state);
72-
static void ExecInitExprSlots(ExprState *state, Node *node);
73-
static void ExecPushExprSlots(ExprState *state, LastAttnumInfo *info);
74-
static bool get_last_attnums_walker(Node *node, LastAttnumInfo *info);
75+
static void ExecCreateExprSetupSteps(ExprState *state, Node *node);
76+
static void ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info);
77+
static bool expr_setup_walker(Node *node, ExprSetupInfo *info);
7578
static bool ExecComputeSlotInfo(ExprState *state, ExprEvalStep *op);
7679
static void ExecInitWholeRowVar(ExprEvalStep *scratch, Var *variable,
7780
ExprState *state);
@@ -140,8 +143,8 @@ ExecInitExpr(Expr *node, PlanState *parent)
140143
state->parent = parent;
141144
state->ext_params = NULL;
142145

143-
/* Insert EEOP_*_FETCHSOME steps as needed */
144-
ExecInitExprSlots(state, (Node *) node);
146+
/* Insert setup steps as needed */
147+
ExecCreateExprSetupSteps(state, (Node *) node);
145148

146149
/* Compile the expression proper */
147150
ExecInitExprRec(node, state, &state->resvalue, &state->resnull);
@@ -177,8 +180,8 @@ ExecInitExprWithParams(Expr *node, ParamListInfo ext_params)
177180
state->parent = NULL;
178181
state->ext_params = ext_params;
179182

180-
/* Insert EEOP_*_FETCHSOME steps as needed */
181-
ExecInitExprSlots(state, (Node *) node);
183+
/* Insert setup steps as needed */
184+
ExecCreateExprSetupSteps(state, (Node *) node);
182185

183186
/* Compile the expression proper */
184187
ExecInitExprRec(node, state, &state->resvalue, &state->resnull);
@@ -232,8 +235,8 @@ ExecInitQual(List *qual, PlanState *parent)
232235
/* mark expression as to be used with ExecQual() */
233236
state->flags = EEO_FLAG_IS_QUAL;
234237

235-
/* Insert EEOP_*_FETCHSOME steps as needed */
236-
ExecInitExprSlots(state, (Node *) qual);
238+
/* Insert setup steps as needed */
239+
ExecCreateExprSetupSteps(state, (Node *) qual);
237240

238241
/*
239242
* ExecQual() needs to return false for an expression returning NULL. That
@@ -376,8 +379,8 @@ ExecBuildProjectionInfo(List *targetList,
376379

377380
state->resultslot = slot;
378381

379-
/* Insert EEOP_*_FETCHSOME steps as needed */
380-
ExecInitExprSlots(state, (Node *) targetList);
382+
/* Insert setup steps as needed */
383+
ExecCreateExprSetupSteps(state, (Node *) targetList);
381384

382385
/* Now compile each tlist column */
383386
foreach(lc, targetList)
@@ -528,7 +531,7 @@ ExecBuildUpdateProjection(List *targetList,
528531
int nAssignableCols;
529532
bool sawJunk;
530533
Bitmapset *assignedCols;
531-
LastAttnumInfo deform = {0, 0, 0};
534+
ExprSetupInfo deform = {0, 0, 0, NIL};
532535
ExprEvalStep scratch = {0};
533536
int outerattnum;
534537
ListCell *lc,
@@ -607,17 +610,18 @@ ExecBuildUpdateProjection(List *targetList,
607610
* number of columns of the "outer" tuple.
608611
*/
609612
if (evalTargetList)
610-
get_last_attnums_walker((Node *) targetList, &deform);
613+
expr_setup_walker((Node *) targetList, &deform);
611614
else
612615
deform.last_outer = nAssignableCols;
613616

614-
ExecPushExprSlots(state, &deform);
617+
ExecPushExprSetupSteps(state, &deform);
615618

616619
/*
617620
* Now generate code to evaluate the tlist's assignable expressions or
618621
* fetch them from the outer tuple, incidentally validating that they'll
619622
* be of the right data type. The checks above ensure that the forboth()
620-
* will iterate over exactly the non-junk columns.
623+
* will iterate over exactly the non-junk columns. Note that we don't
624+
* bother evaluating any remaining resjunk columns.
621625
*/
622626
outerattnum = 0;
623627
forboth(lc, targetList, lc2, targetColnos)
@@ -680,22 +684,6 @@ ExecBuildUpdateProjection(List *targetList,
680684
outerattnum++;
681685
}
682686

683-
/*
684-
* If we're evaluating the tlist, must evaluate any resjunk columns too.
685-
* (This matters for things like MULTIEXPR_SUBLINK SubPlans.)
686-
*/
687-
if (evalTargetList)
688-
{
689-
for_each_cell(lc, targetList, lc)
690-
{
691-
TargetEntry *tle = lfirst_node(TargetEntry, lc);
692-
693-
Assert(tle->resjunk);
694-
ExecInitExprRec(tle->expr, state,
695-
&state->resvalue, &state->resnull);
696-
}
697-
}
698-
699687
/*
700688
* Now generate code to copy over any old columns that were not assigned
701689
* to, and to ensure that dropped columns are set to NULL.
@@ -1406,6 +1394,21 @@ ExecInitExprRec(Expr *node, ExprState *state,
14061394
SubPlan *subplan = (SubPlan *) node;
14071395
SubPlanState *sstate;
14081396

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

@@ -2612,36 +2615,38 @@ ExecInitFunc(ExprEvalStep *scratch, Expr *node, List *args, Oid funcid,
26122615
}
26132616

26142617
/*
2615-
* Add expression steps deforming the ExprState's inner/outer/scan slots
2616-
* as much as required by the expression.
2618+
* Add expression steps performing setup that's needed before any of the
2619+
* main execution of the expression.
26172620
*/
26182621
static void
2619-
ExecInitExprSlots(ExprState *state, Node *node)
2622+
ExecCreateExprSetupSteps(ExprState *state, Node *node)
26202623
{
2621-
LastAttnumInfo info = {0, 0, 0};
2624+
ExprSetupInfo info = {0, 0, 0, NIL};
26222625

2623-
/*
2624-
* Figure out which attributes we're going to need.
2625-
*/
2626-
get_last_attnums_walker(node, &info);
2626+
/* Prescan to find out what we need. */
2627+
expr_setup_walker(node, &info);
26272628

2628-
ExecPushExprSlots(state, &info);
2629+
/* And generate those steps. */
2630+
ExecPushExprSetupSteps(state, &info);
26292631
}
26302632

26312633
/*
2632-
* Add steps deforming the ExprState's inner/out/scan slots as much as
2633-
* indicated by info. This is useful when building an ExprState covering more
2634-
* than one expression.
2634+
* Add steps performing expression setup as indicated by "info".
2635+
* This is useful when building an ExprState covering more than one expression.
26352636
*/
26362637
static void
2637-
ExecPushExprSlots(ExprState *state, LastAttnumInfo *info)
2638+
ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info)
26382639
{
26392640
ExprEvalStep scratch = {0};
2641+
ListCell *lc;
26402642

26412643
scratch.resvalue = NULL;
26422644
scratch.resnull = NULL;
26432645

2644-
/* Emit steps as needed */
2646+
/*
2647+
* Add steps deforming the ExprState's inner/outer/scan slots as much as
2648+
* required by any Vars appearing in the expression.
2649+
*/
26452650
if (info->last_inner > 0)
26462651
{
26472652
scratch.opcode = EEOP_INNER_FETCHSOME;
@@ -2672,13 +2677,48 @@ ExecPushExprSlots(ExprState *state, LastAttnumInfo *info)
26722677
if (ExecComputeSlotInfo(state, &scratch))
26732678
ExprEvalPushStep(state, &scratch);
26742679
}
2680+
2681+
/*
2682+
* Add steps to execute any MULTIEXPR SubPlans appearing in the
2683+
* expression. We need to evaluate these before any of the Params
2684+
* referencing their outputs are used, but after we've prepared for any
2685+
* Var references they may contain. (There cannot be cross-references
2686+
* between MULTIEXPR SubPlans, so we needn't worry about their order.)
2687+
*/
2688+
foreach(lc, info->multiexpr_subplans)
2689+
{
2690+
SubPlan *subplan = (SubPlan *) lfirst(lc);
2691+
SubPlanState *sstate;
2692+
2693+
Assert(subplan->subLinkType == MULTIEXPR_SUBLINK);
2694+
2695+
/* This should match what ExecInitExprRec does for other SubPlans: */
2696+
2697+
if (!state->parent)
2698+
elog(ERROR, "SubPlan found with no parent plan");
2699+
2700+
sstate = ExecInitSubPlan(subplan, state->parent);
2701+
2702+
/* add SubPlanState nodes to state->parent->subPlan */
2703+
state->parent->subPlan = lappend(state->parent->subPlan,
2704+
sstate);
2705+
2706+
scratch.opcode = EEOP_SUBPLAN;
2707+
scratch.d.subplan.sstate = sstate;
2708+
2709+
/* The result can be ignored, but we better put it somewhere */
2710+
scratch.resvalue = &state->resvalue;
2711+
scratch.resnull = &state->resnull;
2712+
2713+
ExprEvalPushStep(state, &scratch);
2714+
}
26752715
}
26762716

26772717
/*
2678-
* get_last_attnums_walker: expression walker for ExecInitExprSlots
2718+
* expr_setup_walker: expression walker for ExecCreateExprSetupSteps
26792719
*/
26802720
static bool
2681-
get_last_attnums_walker(Node *node, LastAttnumInfo *info)
2721+
expr_setup_walker(Node *node, ExprSetupInfo *info)
26822722
{
26832723
if (node == NULL)
26842724
return false;
@@ -2706,6 +2746,16 @@ get_last_attnums_walker(Node *node, LastAttnumInfo *info)
27062746
return false;
27072747
}
27082748

2749+
/* Collect all MULTIEXPR SubPlans, too */
2750+
if (IsA(node, SubPlan))
2751+
{
2752+
SubPlan *subplan = (SubPlan *) node;
2753+
2754+
if (subplan->subLinkType == MULTIEXPR_SUBLINK)
2755+
info->multiexpr_subplans = lappend(info->multiexpr_subplans,
2756+
subplan);
2757+
}
2758+
27092759
/*
27102760
* Don't examine the arguments or filters of Aggrefs or WindowFuncs,
27112761
* because those do not represent expressions to be evaluated within the
@@ -2718,7 +2768,7 @@ get_last_attnums_walker(Node *node, LastAttnumInfo *info)
27182768
return false;
27192769
if (IsA(node, GroupingFunc))
27202770
return false;
2721-
return expression_tree_walker(node, get_last_attnums_walker,
2771+
return expression_tree_walker(node, expr_setup_walker,
27222772
(void *) info);
27232773
}
27242774

@@ -3337,7 +3387,7 @@ ExecBuildAggTrans(AggState *aggstate, AggStatePerPhase phase,
33373387
PlanState *parent = &aggstate->ss.ps;
33383388
ExprEvalStep scratch = {0};
33393389
bool isCombine = DO_AGGSPLIT_COMBINE(aggstate->aggsplit);
3340-
LastAttnumInfo deform = {0, 0, 0};
3390+
ExprSetupInfo deform = {0, 0, 0, NIL};
33413391

33423392
state->expr = (Expr *) aggstate;
33433393
state->parent = parent;
@@ -3353,18 +3403,18 @@ ExecBuildAggTrans(AggState *aggstate, AggStatePerPhase phase,
33533403
{
33543404
AggStatePerTrans pertrans = &aggstate->pertrans[transno];
33553405

3356-
get_last_attnums_walker((Node *) pertrans->aggref->aggdirectargs,
3357-
&deform);
3358-
get_last_attnums_walker((Node *) pertrans->aggref->args,
3359-
&deform);
3360-
get_last_attnums_walker((Node *) pertrans->aggref->aggorder,
3361-
&deform);
3362-
get_last_attnums_walker((Node *) pertrans->aggref->aggdistinct,
3363-
&deform);
3364-
get_last_attnums_walker((Node *) pertrans->aggref->aggfilter,
3365-
&deform);
3406+
expr_setup_walker((Node *) pertrans->aggref->aggdirectargs,
3407+
&deform);
3408+
expr_setup_walker((Node *) pertrans->aggref->args,
3409+
&deform);
3410+
expr_setup_walker((Node *) pertrans->aggref->aggorder,
3411+
&deform);
3412+
expr_setup_walker((Node *) pertrans->aggref->aggdistinct,
3413+
&deform);
3414+
expr_setup_walker((Node *) pertrans->aggref->aggfilter,
3415+
&deform);
33663416
}
3367-
ExecPushExprSlots(state, &deform);
3417+
ExecPushExprSetupSteps(state, &deform);
33683418

33693419
/*
33703420
* Emit instructions for each transition value / grouping set combination.

0 commit comments

Comments
 (0)