Fix MULTIEXPR_SUBLINK with partitioned target tables, yet again.

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/17800-ff90866b3906c964@postgresql.org

1 files changed, 112 insertions, 62 deletions

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 502445acecb..d89445ee7ba 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -51,12 +51,15 @@
 #include "utils/typcache.h"
 
 
-typedef struct LastAttnumInfo
+typedef struct ExprSetupInfo
 {
+	/* Highest attribute numbers fetched from inner/outer/scan tuple slots: */
 	AttrNumber	last_inner;
 	AttrNumber	last_outer;
 	AttrNumber	last_scan;
-} LastAttnumInfo;
+	/* MULTIEXPR SubPlan nodes appearing in the expression: */
+	List	   *multiexpr_subplans;
+} ExprSetupInfo;
 
 static void ExecReadyExpr(ExprState *state);
 static void ExecInitExprRec(Expr *node, ExprState *state,
@@ -64,9 +67,9 @@ static void ExecInitExprRec(Expr *node, ExprState *state,
 static void ExecInitFunc(ExprEvalStep *scratch, Expr *node, List *args,
 						 Oid funcid, Oid inputcollid,
 						 ExprState *state);
-static void ExecInitExprSlots(ExprState *state, Node *node);
-static void ExecPushExprSlots(ExprState *state, LastAttnumInfo *info);
-static bool get_last_attnums_walker(Node *node, LastAttnumInfo *info);
+static void ExecCreateExprSetupSteps(ExprState *state, Node *node);
+static void ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info);
+static bool expr_setup_walker(Node *node, ExprSetupInfo *info);
 static bool ExecComputeSlotInfo(ExprState *state, ExprEvalStep *op);
 static void ExecInitWholeRowVar(ExprEvalStep *scratch, Var *variable,
 								ExprState *state);
@@ -135,8 +138,8 @@ ExecInitExpr(Expr *node, PlanState *parent)
 	state->parent = parent;
 	state->ext_params = NULL;
 
-	/* Insert EEOP_*_FETCHSOME steps as needed */
-	ExecInitExprSlots(state, (Node *) node);
+	/* Insert setup steps as needed */
+	ExecCreateExprSetupSteps(state, (Node *) node);
 
 	/* Compile the expression proper */
 	ExecInitExprRec(node, state, &state->resvalue, &state->resnull);
@@ -172,8 +175,8 @@ ExecInitExprWithParams(Expr *node, ParamListInfo ext_params)
 	state->parent = NULL;
 	state->ext_params = ext_params;
 
-	/* Insert EEOP_*_FETCHSOME steps as needed */
-	ExecInitExprSlots(state, (Node *) node);
+	/* Insert setup steps as needed */
+	ExecCreateExprSetupSteps(state, (Node *) node);
 
 	/* Compile the expression proper */
 	ExecInitExprRec(node, state, &state->resvalue, &state->resnull);
@@ -227,8 +230,8 @@ ExecInitQual(List *qual, PlanState *parent)
 	/* mark expression as to be used with ExecQual() */
 	state->flags = EEO_FLAG_IS_QUAL;
 
-	/* Insert EEOP_*_FETCHSOME steps as needed */
-	ExecInitExprSlots(state, (Node *) qual);
+	/* Insert setup steps as needed */
+	ExecCreateExprSetupSteps(state, (Node *) qual);
 
 	/*
 	 * ExecQual() needs to return false for an expression returning NULL. That
@@ -371,8 +374,8 @@ ExecBuildProjectionInfo(List *targetList,
 
 	state->resultslot = slot;
 
-	/* Insert EEOP_*_FETCHSOME steps as needed */
-	ExecInitExprSlots(state, (Node *) targetList);
+	/* Insert setup steps as needed */
+	ExecCreateExprSetupSteps(state, (Node *) targetList);
 
 	/* Now compile each tlist column */
 	foreach(lc, targetList)
@@ -523,7 +526,7 @@ ExecBuildUpdateProjection(List *targetList,
 	int			nAssignableCols;
 	bool		sawJunk;
 	Bitmapset  *assignedCols;
-	LastAttnumInfo deform = {0, 0, 0};
+	ExprSetupInfo deform = {0, 0, 0, NIL};
 	ExprEvalStep scratch = {0};
 	int			outerattnum;
 	ListCell   *lc,
@@ -602,17 +605,18 @@ ExecBuildUpdateProjection(List *targetList,
 	 * number of columns of the "outer" tuple.
 	 */
 	if (evalTargetList)
-		get_last_attnums_walker((Node *) targetList, &deform);
+		expr_setup_walker((Node *) targetList, &deform);
 	else
 		deform.last_outer = nAssignableCols;
 
-	ExecPushExprSlots(state, &deform);
+	ExecPushExprSetupSteps(state, &deform);
 
 	/*
 	 * Now generate code to evaluate the tlist's assignable expressions or
 	 * fetch them from the outer tuple, incidentally validating that they'll
 	 * be of the right data type.  The checks above ensure that the forboth()
-	 * will iterate over exactly the non-junk columns.
+	 * will iterate over exactly the non-junk columns.  Note that we don't
+	 * bother evaluating any remaining resjunk columns.
 	 */
 	outerattnum = 0;
 	forboth(lc, targetList, lc2, targetColnos)
@@ -676,22 +680,6 @@ ExecBuildUpdateProjection(List *targetList,
 	}
 
 	/*
-	 * If we're evaluating the tlist, must evaluate any resjunk columns too.
-	 * (This matters for things like MULTIEXPR_SUBLINK SubPlans.)
-	 */
-	if (evalTargetList)
-	{
-		for_each_cell(lc, targetList, lc)
-		{
-			TargetEntry *tle = lfirst_node(TargetEntry, lc);
-
-			Assert(tle->resjunk);
-			ExecInitExprRec(tle->expr, state,
-							&state->resvalue, &state->resnull);
-		}
-	}
-
-	/*
 	 * Now generate code to copy over any old columns that were not assigned
 	 * to, and to ensure that dropped columns are set to NULL.
 	 */
@@ -1401,6 +1389,21 @@ ExecInitExprRec(Expr *node, ExprState *state,
 				SubPlan    *subplan = (SubPlan *) node;
 				SubPlanState *sstate;
 
+				/*
+				 * Real execution of a MULTIEXPR SubPlan has already been
+				 * done. What we have to do here is return a dummy NULL record
+				 * value in case this targetlist element is assigned
+				 * someplace.
+				 */
+				if (subplan->subLinkType == MULTIEXPR_SUBLINK)
+				{
+					scratch.opcode = EEOP_CONST;
+					scratch.d.constval.value = (Datum) 0;
+					scratch.d.constval.isnull = true;
+					ExprEvalPushStep(state, &scratch);
+					break;
+				}
+
 				if (!state->parent)
 					elog(ERROR, "SubPlan found with no parent plan");
 
@@ -2552,36 +2555,38 @@ ExecInitFunc(ExprEvalStep *scratch, Expr *node, List *args, Oid funcid,
 }
 
 /*
- * Add expression steps deforming the ExprState's inner/outer/scan slots
- * as much as required by the expression.
+ * Add expression steps performing setup that's needed before any of the
+ * main execution of the expression.
  */
 static void
-ExecInitExprSlots(ExprState *state, Node *node)
+ExecCreateExprSetupSteps(ExprState *state, Node *node)
 {
-	LastAttnumInfo info = {0, 0, 0};
+	ExprSetupInfo info = {0, 0, 0, NIL};
 
-	/*
-	 * Figure out which attributes we're going to need.
-	 */
-	get_last_attnums_walker(node, &info);
+	/* Prescan to find out what we need. */
+	expr_setup_walker(node, &info);
 
-	ExecPushExprSlots(state, &info);
+	/* And generate those steps. */
+	ExecPushExprSetupSteps(state, &info);
 }
 
 /*
- * Add steps deforming the ExprState's inner/out/scan slots as much as
- * indicated by info. This is useful when building an ExprState covering more
- * than one expression.
+ * Add steps performing expression setup as indicated by "info".
+ * This is useful when building an ExprState covering more than one expression.
  */
 static void
-ExecPushExprSlots(ExprState *state, LastAttnumInfo *info)
+ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info)
 {
 	ExprEvalStep scratch = {0};
+	ListCell   *lc;
 
 	scratch.resvalue = NULL;
 	scratch.resnull = NULL;
 
-	/* Emit steps as needed */
+	/*
+	 * Add steps deforming the ExprState's inner/outer/scan slots as much as
+	 * required by any Vars appearing in the expression.
+	 */
 	if (info->last_inner > 0)
 	{
 		scratch.opcode = EEOP_INNER_FETCHSOME;
@@ -2612,13 +2617,48 @@ ExecPushExprSlots(ExprState *state, LastAttnumInfo *info)
 		if (ExecComputeSlotInfo(state, &scratch))
 			ExprEvalPushStep(state, &scratch);
 	}
+
+	/*
+	 * Add steps to execute any MULTIEXPR SubPlans appearing in the
+	 * expression.  We need to evaluate these before any of the Params
+	 * referencing their outputs are used, but after we've prepared for any
+	 * Var references they may contain.  (There cannot be cross-references
+	 * between MULTIEXPR SubPlans, so we needn't worry about their order.)
+	 */
+	foreach(lc, info->multiexpr_subplans)
+	{
+		SubPlan    *subplan = (SubPlan *) lfirst(lc);
+		SubPlanState *sstate;
+
+		Assert(subplan->subLinkType == MULTIEXPR_SUBLINK);
+
+		/* This should match what ExecInitExprRec does for other SubPlans: */
+
+		if (!state->parent)
+			elog(ERROR, "SubPlan found with no parent plan");
+
+		sstate = ExecInitSubPlan(subplan, state->parent);
+
+		/* add SubPlanState nodes to state->parent->subPlan */
+		state->parent->subPlan = lappend(state->parent->subPlan,
+										 sstate);
+
+		scratch.opcode = EEOP_SUBPLAN;
+		scratch.d.subplan.sstate = sstate;
+
+		/* The result can be ignored, but we better put it somewhere */
+		scratch.resvalue = &state->resvalue;
+		scratch.resnull = &state->resnull;
+
+		ExprEvalPushStep(state, &scratch);
+	}
 }
 
 /*
- * get_last_attnums_walker: expression walker for ExecInitExprSlots
+ * expr_setup_walker: expression walker for ExecCreateExprSetupSteps
  */
 static bool
-get_last_attnums_walker(Node *node, LastAttnumInfo *info)
+expr_setup_walker(Node *node, ExprSetupInfo *info)
 {
 	if (node == NULL)
 		return false;
@@ -2646,6 +2686,16 @@ get_last_attnums_walker(Node *node, LastAttnumInfo *info)
 		return false;
 	}
 
+	/* Collect all MULTIEXPR SubPlans, too */
+	if (IsA(node, SubPlan))
+	{
+		SubPlan    *subplan = (SubPlan *) node;
+
+		if (subplan->subLinkType == MULTIEXPR_SUBLINK)
+			info->multiexpr_subplans = lappend(info->multiexpr_subplans,
+											   subplan);
+	}
+
 	/*
 	 * Don't examine the arguments or filters of Aggrefs or WindowFuncs,
 	 * because those do not represent expressions to be evaluated within the
@@ -2658,7 +2708,7 @@ get_last_attnums_walker(Node *node, LastAttnumInfo *info)
 		return false;
 	if (IsA(node, GroupingFunc))
 		return false;
-	return expression_tree_walker(node, get_last_attnums_walker,
+	return expression_tree_walker(node, expr_setup_walker,
 								  (void *) info);
 }
 
@@ -3277,7 +3327,7 @@ ExecBuildAggTrans(AggState *aggstate, AggStatePerPhase phase,
 	PlanState  *parent = &aggstate->ss.ps;
 	ExprEvalStep scratch = {0};
 	bool		isCombine = DO_AGGSPLIT_COMBINE(aggstate->aggsplit);
-	LastAttnumInfo deform = {0, 0, 0};
+	ExprSetupInfo deform = {0, 0, 0, NIL};
 
 	state->expr = (Expr *) aggstate;
 	state->parent = parent;
@@ -3293,18 +3343,18 @@ ExecBuildAggTrans(AggState *aggstate, AggStatePerPhase phase,
 	{
 		AggStatePerTrans pertrans = &aggstate->pertrans[transno];
 
-		get_last_attnums_walker((Node *) pertrans->aggref->aggdirectargs,
-								&deform);
-		get_last_attnums_walker((Node *) pertrans->aggref->args,
-								&deform);
-		get_last_attnums_walker((Node *) pertrans->aggref->aggorder,
-								&deform);
-		get_last_attnums_walker((Node *) pertrans->aggref->aggdistinct,
-								&deform);
-		get_last_attnums_walker((Node *) pertrans->aggref->aggfilter,
-								&deform);
+		expr_setup_walker((Node *) pertrans->aggref->aggdirectargs,
+						  &deform);
+		expr_setup_walker((Node *) pertrans->aggref->args,
+						  &deform);
+		expr_setup_walker((Node *) pertrans->aggref->aggorder,
+						  &deform);
+		expr_setup_walker((Node *) pertrans->aggref->aggdistinct,
+						  &deform);
+		expr_setup_walker((Node *) pertrans->aggref->aggfilter,
+						  &deform);
 	}
-	ExecPushExprSlots(state, &deform);
+	ExecPushExprSetupSteps(state, &deform);
 
 	/*
 	 * Emit instructions for each transition value / grouping set combination.