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, 67 insertions, 67 deletions

diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 64af36a1873..c136f75ac24 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -235,47 +235,6 @@ ExecScanSubPlan(SubPlanState *node,
 	ListCell   *l;
 	ArrayBuildStateAny *astate = NULL;
 
-	/*
-	 * MULTIEXPR subplans, when "executed", just return NULL; but first we
-	 * mark the subplan's output parameters as needing recalculation.  (This
-	 * is a bit of a hack: it relies on the subplan appearing later in its
-	 * targetlist than any of the referencing Params, so that all the Params
-	 * have been evaluated before we re-mark them for the next evaluation
-	 * cycle.  But in general resjunk tlist items appear after non-resjunk
-	 * ones, so this should be safe.)  Unlike ExecReScanSetParamPlan, we do
-	 * *not* set bits in the parent plan node's chgParam, because we don't
-	 * want to cause a rescan of the parent.
-	 *
-	 * Note: we are also relying on MULTIEXPR SubPlans not sharing any output
-	 * parameters with other SubPlans, because if one does then it is unclear
-	 * which SubPlanState node the parameter's execPlan field will be pointing
-	 * to when we come to evaluate the parameter.  We can allow plain initplan
-	 * SubPlans to share output parameters, because it doesn't actually matter
-	 * which initplan SubPlan we reference as long as they all point to the
-	 * same underlying subplan.  However, that fails to hold for MULTIEXPRs
-	 * because they can have non-empty args lists, and the "same" args might
-	 * have mutated into different forms in different parts of a plan tree.
-	 * There is currently no problem because MULTIEXPR can appear only in an
-	 * UPDATE's top-level target list, so it won't get duplicated anyplace.
-	 * Postgres versions before v14 had to make concrete efforts to avoid
-	 * sharing output parameters across different clones of a MULTIEXPR, and
-	 * the problem could recur someday.
-	 */
-	if (subLinkType == MULTIEXPR_SUBLINK)
-	{
-		EState	   *estate = node->parent->state;
-
-		foreach(l, subplan->setParam)
-		{
-			int			paramid = lfirst_int(l);
-			ParamExecData *prm = &(estate->es_param_exec_vals[paramid]);
-
-			prm->execPlan = node;
-		}
-		*isNull = true;
-		return (Datum) 0;
-	}
-
 	/* Initialize ArrayBuildStateAny in caller's context, if needed */
 	if (subLinkType == ARRAY_SUBLINK)
 		astate = initArrayResultAny(subplan->firstColType,
@@ -327,6 +286,11 @@ ExecScanSubPlan(SubPlanState *node,
 	 * NULL.  Assuming we get a tuple, we just use its first column (there can
 	 * be only one non-junk column in this case).
 	 *
+	 * For MULTIEXPR_SUBLINK, we push the per-column subplan outputs out to
+	 * the setParams and then return a dummy false value.  There must not be
+	 * multiple tuples returned from the subplan; if zero tuples are produced,
+	 * set the setParams to NULL.
+	 *
 	 * For ARRAY_SUBLINK we allow the subplan to produce any number of tuples,
 	 * and form an array of the first column's values.  Note in particular
 	 * that we produce a zero-element array if no tuples are produced (this is
@@ -378,6 +342,47 @@ ExecScanSubPlan(SubPlanState *node,
 			continue;
 		}
 
+		if (subLinkType == MULTIEXPR_SUBLINK)
+		{
+			/* cannot allow multiple input tuples for MULTIEXPR sublink */
+			if (found)
+				ereport(ERROR,
+						(errcode(ERRCODE_CARDINALITY_VIOLATION),
+						 errmsg("more than one row returned by a subquery used as an expression")));
+			found = true;
+
+			/*
+			 * We need to copy the subplan's tuple in case any result is of
+			 * pass-by-ref type --- our output values will point into this
+			 * copied tuple!  Can't use the subplan's instance of the tuple
+			 * since it won't still be valid after next ExecProcNode() call.
+			 * node->curTuple keeps track of the copied tuple for eventual
+			 * freeing.
+			 */
+			if (node->curTuple)
+				heap_freetuple(node->curTuple);
+			node->curTuple = ExecCopySlotHeapTuple(slot);
+
+			/*
+			 * Now set all the setParam params from the columns of the tuple
+			 */
+			col = 1;
+			foreach(plst, subplan->setParam)
+			{
+				int			paramid = lfirst_int(plst);
+				ParamExecData *prmdata;
+
+				prmdata = &(econtext->ecxt_param_exec_vals[paramid]);
+				Assert(prmdata->execPlan == NULL);
+				prmdata->value = heap_getattr(node->curTuple, col, tdesc,
+											  &(prmdata->isnull));
+				col++;
+			}
+
+			/* keep scanning subplan to make sure there's only one tuple */
+			continue;
+		}
+
 		if (subLinkType == ARRAY_SUBLINK)
 		{
 			Datum		dvalue;
@@ -473,6 +478,20 @@ ExecScanSubPlan(SubPlanState *node,
 			result = (Datum) 0;
 			*isNull = true;
 		}
+		else if (subLinkType == MULTIEXPR_SUBLINK)
+		{
+			/* We don't care about function result, but set the setParams */
+			foreach(l, subplan->setParam)
+			{
+				int			paramid = lfirst_int(l);
+				ParamExecData *prmdata;
+
+				prmdata = &(econtext->ecxt_param_exec_vals[paramid]);
+				Assert(prmdata->execPlan == NULL);
+				prmdata->value = (Datum) 0;
+				prmdata->isnull = true;
+			}
+		}
 	}
 
 	return result;
@@ -848,10 +867,9 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
 	sstate->cur_eq_funcs = NULL;
 
 	/*
-	 * If this is an initplan or MULTIEXPR subplan, it has output parameters
-	 * that the parent plan will use, so mark those parameters as needing
-	 * evaluation.  We don't actually run the subplan until we first need one
-	 * of its outputs.
+	 * If this is an initplan, it has output parameters that the parent plan
+	 * will use, so mark those parameters as needing evaluation.  We don't
+	 * actually run the subplan until we first need one of its outputs.
 	 *
 	 * A CTE subplan's output parameter is never to be evaluated in the normal
 	 * way, so skip this in that case.
@@ -859,7 +877,8 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
 	 * Note that we don't set parent->chgParam here: the parent plan hasn't
 	 * been run yet, so no need to force it to re-run.
 	 */
-	if (subplan->setParam != NIL && subplan->subLinkType != CTE_SUBLINK)
+	if (subplan->setParam != NIL && subplan->parParam == NIL &&
+		subplan->subLinkType != CTE_SUBLINK)
 	{
 		ListCell   *lst;
 
@@ -1079,7 +1098,6 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
 	ScanDirection dir = estate->es_direction;
 	MemoryContext oldcontext;
 	TupleTableSlot *slot;
-	ListCell   *pvar;
 	ListCell   *l;
 	bool		found = false;
 	ArrayBuildStateAny *astate = NULL;
@@ -1089,6 +1107,8 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
 		elog(ERROR, "ANY/ALL subselect unsupported as initplan");
 	if (subLinkType == CTE_SUBLINK)
 		elog(ERROR, "CTE subplans should not be executed via ExecSetParamPlan");
+	if (subplan->parParam || node->args)
+		elog(ERROR, "correlated subplans should not be executed via ExecSetParamPlan");
 
 	/*
 	 * Enforce forward scan direction regardless of caller. It's hard but not
@@ -1107,26 +1127,6 @@ ExecSetParamPlan(SubPlanState *node, ExprContext *econtext)
 	oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
 
 	/*
-	 * Set Params of this plan from parent plan correlation values. (Any
-	 * calculation we have to do is done in the parent econtext, since the
-	 * Param values don't need to have per-query lifetime.)  Currently, we
-	 * expect only MULTIEXPR_SUBLINK plans to have any correlation values.
-	 */
-	Assert(subplan->parParam == NIL || subLinkType == MULTIEXPR_SUBLINK);
-	Assert(list_length(subplan->parParam) == list_length(node->args));
-
-	forboth(l, subplan->parParam, pvar, node->args)
-	{
-		int			paramid = lfirst_int(l);
-		ParamExecData *prm = &(econtext->ecxt_param_exec_vals[paramid]);
-
-		prm->value = ExecEvalExprSwitchContext((ExprState *) lfirst(pvar),
-											   econtext,
-											   &(prm->isnull));
-		planstate->chgParam = bms_add_member(planstate->chgParam, paramid);
-	}
-
-	/*
 	 * Run the plan.  (If it needs to be rescanned, the first ExecProcNode
 	 * call will take care of that.)
 	 */