pg.ddx.io pgsql-bugs@postgresql.org mailing list archive help / color / mirror / Atom feed From: Andres Freund <andres@anarazel.de>
Subject: [PATCH v2] WIP: generate explicit expression step to evaluate correlated MULTIEXPR_SUBLINK
Date: Thu, 23 Feb 2023 17:48:12 -0800
---
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(-)
inline patch 1: src/include/executor/execExpr.h, src/include/executor/nodeSubplan.h, src/backend/executor/execExpr.c and 4 more (+137/-2 lines) download inline-1.patch 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
Message-ID: <no-message-id-28036@localhost>
Permalink: ../no-message-id-28036@localhost/
Also on: postgresql.org/message-id/no-message-id-28036@localhost
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: andres@anarazel.de
Subject: Re: [PATCH v2] WIP: generate explicit expression step to evaluate correlated MULTIEXPR_SUBLINK
In-Reply-To: <no-message-id-28036@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox