From 1aed0dc46f63b1332dfaad9b40a8c08250227b87 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Thu, 23 Feb 2023 17:48:12 -0800
Subject: [PATCH v2] WIP: generate explicit expression step to evaluate
 correlated MULTIEXPR_SUBLINK

---
 src/include/executor/execExpr.h       |  6 ++-
 src/include/executor/nodeSubplan.h    |  2 +
 src/backend/executor/execExpr.c       | 78 +++++++++++++++++++++++++++
 src/backend/executor/execExprInterp.c | 21 ++++++++
 src/backend/executor/nodeSubplan.c    | 25 ++++++++-
 src/backend/jit/llvm/llvmjit_expr.c   |  6 +++
 src/backend/jit/llvm/llvmjit_types.c  |  1 +
 7 files changed, 137 insertions(+), 2 deletions(-)

diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index 06c3adc0a19..faa806e1a16 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -256,6 +256,8 @@ typedef enum ExprEvalOp
 	EEOP_AGG_ORDERED_TRANS_DATUM,
 	EEOP_AGG_ORDERED_TRANS_TUPLE,
 
+	EEOP_SUBPLAN_PARAMS,
+
 	/* non-existent operation, used e.g. to check array lengths */
 	EEOP_LAST
 } ExprEvalOp;
@@ -607,7 +609,7 @@ typedef struct ExprEvalStep
 			WindowFuncExprState *wfstate;
 		}			window_func;
 
-		/* for EEOP_SUBPLAN */
+		/* for EEOP_SUBPLAN and EEOP_SUBPLAN_PARAMS */
 		struct
 		{
 			/* out-of-line state, created by nodeSubplan.c */
@@ -766,6 +768,8 @@ extern void ExecEvalXmlExpr(ExprState *state, ExprEvalStep *op);
 extern void ExecEvalGroupingFunc(ExprState *state, ExprEvalStep *op);
 extern void ExecEvalSubPlan(ExprState *state, ExprEvalStep *op,
 							ExprContext *econtext);
+extern void ExecEvalSubPlanParams(ExprState *state, ExprEvalStep *op,
+								  ExprContext *econtext);
 extern void ExecEvalWholeRowVar(ExprState *state, ExprEvalStep *op,
 								ExprContext *econtext);
 extern void ExecEvalSysVar(ExprState *state, ExprEvalStep *op,
diff --git a/src/include/executor/nodeSubplan.h b/src/include/executor/nodeSubplan.h
index 3f0e2bba2b7..cf369c239a6 100644
--- a/src/include/executor/nodeSubplan.h
+++ b/src/include/executor/nodeSubplan.h
@@ -26,4 +26,6 @@ extern void ExecSetParamPlan(SubPlanState *node, ExprContext *econtext);
 
 extern void ExecSetParamPlanMulti(const Bitmapset *params, ExprContext *econtext);
 
+extern bool IsCorrelatedMultiSubPlan(SubPlan *subplan);
+
 #endif							/* NODESUBPLAN_H */
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 812ead95bc6..b176001bea8 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -52,11 +52,13 @@
 #include "utils/typcache.h"
 
 
+/* FIXME: rename */
 typedef struct LastAttnumInfo
 {
 	AttrNumber	last_inner;
 	AttrNumber	last_outer;
 	AttrNumber	last_scan;
+	List *multiexpr_subplans;
 } LastAttnumInfo;
 
 static void ExecReadyExpr(ExprState *state);
@@ -444,6 +446,12 @@ ExecBuildProjectionInfo(List *targetList,
 			scratch.d.assign_var.resultnum = tle->resno - 1;
 			ExprEvalPushStep(state, &scratch);
 		}
+		else if (IsA(tle->expr, SubPlan) &&
+				 IsCorrelatedMultiSubPlan((SubPlan *) tle->expr))
+		{
+			/* correlated multiexprs already have been handled */
+			continue;
+		}
 		else
 		{
 			/*
@@ -679,6 +687,9 @@ ExecBuildUpdateProjection(List *targetList,
 	/*
 	 * If we're evaluating the tlist, must evaluate any resjunk columns too.
 	 * (This matters for things like MULTIEXPR_SUBLINK SubPlans.)
+	 *
+	 * FIXME: Are there any cases where we still need to do this, given the
+	 * multiexpr case is already handled?
 	 */
 	if (evalTargetList)
 	{
@@ -687,6 +698,12 @@ ExecBuildUpdateProjection(List *targetList,
 			TargetEntry *tle = lfirst_node(TargetEntry, lc);
 
 			Assert(tle->resjunk);
+
+			/* correlated multiexprs already have been handled */
+			if (tle->resjunk && IsA(tle->expr, SubPlan) &&
+				IsCorrelatedMultiSubPlan((SubPlan *) tle->expr))
+				continue;
+
 			ExecInitExprRec(tle->expr, state,
 							&state->resvalue, &state->resnull);
 		}
@@ -1405,6 +1422,9 @@ ExecInitExprRec(Expr *node, ExprState *state,
 				if (!state->parent)
 					elog(ERROR, "SubPlan found with no parent plan");
 
+				/* needs to already have been evaluated */
+				Assert(!IsCorrelatedMultiSubPlan(subplan));
+
 				sstate = ExecInitSubPlan(subplan, state->parent);
 
 				/* add SubPlanState nodes to state->parent->subPlan */
@@ -2544,6 +2564,8 @@ 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.
+ *
+ * FIXME: rename
  */
 static void
 ExecInitExprSlots(ExprState *state, Node *node)
@@ -2562,11 +2584,14 @@ ExecInitExprSlots(ExprState *state, Node *node)
  * 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.
+ *
+ * FIXME: rename
  */
 static void
 ExecPushExprSlots(ExprState *state, LastAttnumInfo *info)
 {
 	ExprEvalStep scratch = {0};
+	ListCell *lc;
 
 	scratch.resvalue = NULL;
 	scratch.resnull = NULL;
@@ -2602,10 +2627,37 @@ ExecPushExprSlots(ExprState *state, LastAttnumInfo *info)
 		if (ExecComputeSlotInfo(state, &scratch))
 			ExprEvalPushStep(state, &scratch);
 	}
+
+	/*
+	 * FIXME: This obviously isn't the right place, but for some reason
+	 * ExecBuildUpdateProjection() duplicates the get_last_attnums_walker
+	 * invocation.
+	 */
+	foreach(lc, info->multiexpr_subplans)
+	{
+		SubPlan *sp = (SubPlan *) lfirst(lc);
+		SubPlanState *sstate;
+
+		if (!state->parent)
+			elog(ERROR, "SubPlan found with no parent plan");
+
+		sstate = ExecInitSubPlan(sp, state->parent);
+
+		/* add SubPlanState nodes to state->parent->subPlan */
+		state->parent->subPlan = lappend(state->parent->subPlan,
+										 sstate);
+
+		scratch.opcode = EEOP_SUBPLAN_PARAMS;
+		scratch.d.subplan.sstate = sstate;
+		ExprEvalPushStep(state, &scratch);
+	}
+
 }
 
 /*
  * get_last_attnums_walker: expression walker for ExecInitExprSlots
+ *
+ * FIXME: rename
  */
 static bool
 get_last_attnums_walker(Node *node, LastAttnumInfo *info)
@@ -2636,6 +2688,32 @@ get_last_attnums_walker(Node *node, LastAttnumInfo *info)
 		return false;
 	}
 
+	/*
+	 * Collect all MULTIEXPR subplans. We want generate SUBPLAN_PARAMS steps
+	 * before we could encounter a Param for the subplan. That can
+	 * conveniently be done here, since we already do a full expression tree
+	 * traversal.
+	 *
+	 * FIXME: Is it worth building bitmasks of used paramids to avoid
+	 * unnecessary evaluation of a SubPlan? I don't think they should be
+	 * emitted into the targetlist if not referenced?
+	 *
+	 * FIXME: I think this may make sense for initplans as well, not just for
+	 * the MULTIEXPR case.
+	 *
+	 * XXX: This is probably worth doing for other kinds of subplans as well.
+	 */
+	if (IsA(node, SubPlan))
+	{
+		SubPlan *subplan = (SubPlan *) node;
+
+		if (IsCorrelatedMultiSubPlan(subplan))
+		{
+			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
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 19351fe34bf..ebae1ef423d 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -493,6 +493,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
 		&&CASE_EEOP_AGG_PRESORTED_DISTINCT_MULTI,
 		&&CASE_EEOP_AGG_ORDERED_TRANS_DATUM,
 		&&CASE_EEOP_AGG_ORDERED_TRANS_TUPLE,
+		&&CASE_EEOP_SUBPLAN_PARAMS,
 		&&CASE_EEOP_LAST
 	};
 
@@ -1804,6 +1805,14 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
 			EEO_NEXT();
 		}
 
+		EEO_CASE(EEOP_SUBPLAN_PARAMS)
+		{
+			/* too complex for an inline implementation */
+			ExecEvalSubPlanParams(state, op, econtext);
+
+			EEO_NEXT();
+		}
+
 		EEO_CASE(EEOP_LAST)
 		{
 			/* unreachable */
@@ -3912,6 +3921,18 @@ ExecEvalSubPlan(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
 	*op->resvalue = ExecSubPlan(sstate, econtext, op->resnull);
 }
 
+void
+ExecEvalSubPlanParams(ExprState *state, ExprEvalStep *op,
+					  ExprContext *econtext)
+{
+	SubPlanState *sstate = op->d.subplan.sstate;
+
+	/* could potentially be nested, so make sure there's enough stack */
+	check_stack_depth();
+
+	ExecSetParamPlan(sstate, econtext);
+}
+
 /*
  * Evaluate a wholerow Var expression.
  *
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 64af36a1873..8d0c9a79a3b 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -265,6 +265,13 @@ ExecScanSubPlan(SubPlanState *node,
 	{
 		EState	   *estate = node->parent->state;
 
+		/*
+		 * ExecScanSubPlan() shouldn't be called for correlated subqueries,
+		 * execExpr should have handled the case internally. Note we may get
+		 * called for non-correlated MULTIEXPR.
+		 */
+		Assert(!IsCorrelatedMultiSubPlan(subplan));
+
 		foreach(l, subplan->setParam)
 		{
 			int			paramid = lfirst_int(l);
@@ -868,7 +875,9 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
 			int			paramid = lfirst_int(lst);
 			ParamExecData *prm = &(estate->es_param_exec_vals[paramid]);
 
-			prm->execPlan = sstate;
+			/* MULTIEXPR gets automatically invoked when necessary via execExpr.c  */
+			if (!IsCorrelatedMultiSubPlan(subplan))
+				prm->execPlan = sstate;
 		}
 	}
 
@@ -1326,3 +1335,17 @@ ExecReScanSetParamPlan(SubPlanState *node, PlanState *parent)
 		parent->chgParam = bms_add_member(parent->chgParam, paramid);
 	}
 }
+
+/*
+ * XXX: Where should this be? Perhaps SubPlan should just indicate this more
+ * conveniently?
+ */
+bool
+IsCorrelatedMultiSubPlan(SubPlan *subplan)
+{
+	if (subplan->subLinkType != MULTIEXPR_SUBLINK)
+		return false;
+
+	/* can't be correlated if there are no input parameters */
+	return subplan->parParam != NULL;
+}
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 1c722c79552..70d8c328255 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1904,6 +1904,12 @@ llvm_compile_expr(ExprState *state)
 				LLVMBuildBr(b, opblocks[opno + 1]);
 				break;
 
+			case EEOP_SUBPLAN_PARAMS:
+				build_EvalXFunc(b, mod, "ExecEvalSubPlanParams",
+								v_state, op, v_econtext);
+				LLVMBuildBr(b, opblocks[opno + 1]);
+				break;
+
 			case EEOP_AGG_STRICT_DESERIALIZE:
 			case EEOP_AGG_DESERIALIZE:
 				{
diff --git a/src/backend/jit/llvm/llvmjit_types.c b/src/backend/jit/llvm/llvmjit_types.c
index 876fb640294..048029c5d0b 100644
--- a/src/backend/jit/llvm/llvmjit_types.c
+++ b/src/backend/jit/llvm/llvmjit_types.c
@@ -129,6 +129,7 @@ void	   *referenced_functions[] =
 	ExecEvalScalarArrayOp,
 	ExecEvalHashedScalarArrayOp,
 	ExecEvalSubPlan,
+	ExecEvalSubPlanParams,
 	ExecEvalSysVar,
 	ExecEvalWholeRowVar,
 	ExecEvalXmlExpr,
-- 
2.38.0

