Evaluate arguments of correlated SubPlans in the referencing ExprState
Hi,
Around
/messages/by-id/20230224015417.75yimxbksejpffh3@awork3.anarazel.de
I suggested that we should evaluate the arguments of correlated SubPlans as
part of the expression referencing the subplan.
Here's a patch for that.
Ended up simpler than I'd thought. I see small, consistent, speedups and
reductions in memory usage.
I think individual arguments are mainly (always?) Var nodes. By evaluating
them as part of the containing expression we avoid the increased memory usage,
and the increased dispatch of going through another layer of
ExprState. Because the arguments are a single Var, which end up with a
slot_getattr() via ExecJust*Var, we also elide redundant slot_getattr()
checks. I think we already avoided redundant tuple deforming, because the
parent ExprState will have done that already.
Greetings,
Andres Freund
Attachments:
v1-0001-WIP-Evaluate-arguments-of-correlated-SubPlans-in-.patchtext/x-diff; charset=us-asciiDownload
From 3b68577bbcd0f78b80abe1ac07eedd6998254951 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sat, 25 Feb 2023 13:39:19 -0800
Subject: [PATCH v1] WIP: Evaluate arguments of correlated SubPlans in the
referencing ExprState
---
src/include/nodes/execnodes.h | 1 -
src/backend/executor/execExpr.c | 81 +++++++++++++++++------------
src/backend/executor/execProcnode.c | 5 ++
src/backend/executor/nodeSubplan.c | 30 +++++------
4 files changed, 66 insertions(+), 51 deletions(-)
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 20f4c8b35f3..437cf8b5a02 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -947,7 +947,6 @@ typedef struct SubPlanState
struct PlanState *planstate; /* subselect plan's state tree */
struct PlanState *parent; /* parent plan node's state tree */
ExprState *testexpr; /* state of combining expression */
- List *args; /* states of argument expression(s) */
HeapTuple curTuple; /* copy of most recent tuple from subplan */
Datum curArray; /* most recent array from ARRAY() subplan */
/* these are used when hashing the subselect's output: */
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index c61f23c6c18..7a9d5729b4b 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -87,6 +87,9 @@ static void ExecBuildAggTransCall(ExprState *state, AggState *aggstate,
FunctionCallInfo fcinfo, AggStatePerTrans pertrans,
int transno, int setno, int setoff, bool ishash,
bool nullcheck);
+static void ExecInitSubPlanExpr(SubPlan *subplan,
+ ExprState *state,
+ Datum *resv, bool *resnull);
/*
@@ -1388,7 +1391,6 @@ ExecInitExprRec(Expr *node, ExprState *state,
case T_SubPlan:
{
SubPlan *subplan = (SubPlan *) node;
- SubPlanState *sstate;
/*
* Real execution of a MULTIEXPR SubPlan has already been
@@ -1405,19 +1407,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
break;
}
- 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;
-
- ExprEvalPushStep(state, &scratch);
+ ExecInitSubPlanExpr(subplan, state, resv, resnull);
break;
}
@@ -2618,29 +2608,12 @@ ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info)
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);
+ ExecInitSubPlanExpr(subplan, state,
+ &state->resvalue, &state->resnull);
}
}
@@ -4040,3 +4013,45 @@ ExecBuildParamSetEqual(TupleDesc desc,
return state;
}
+
+static void
+ExecInitSubPlanExpr(SubPlan *subplan,
+ ExprState *state,
+ Datum *resv, bool *resnull)
+{
+ ExprEvalStep scratch = {0};
+ SubPlanState *sstate;
+ ListCell *pvar;
+ ListCell *l;
+ EState *estate = state->parent->state;
+
+ if (!state->parent)
+ elog(ERROR, "SubPlan found with no parent plan");
+
+ /*
+ * Generate steps to evaluate input arguments for the subplan.
+ *
+ * Any calculation we have to do can be done in the parent econtext, since
+ * the Param values don't need to have per-query lifetime.
+ */
+ forboth(l, subplan->parParam, pvar, subplan->args)
+ {
+ int paramid = lfirst_int(l);
+ ParamExecData *prm = &estate->es_param_exec_vals[paramid];
+
+ ExecInitExprRec(lfirst(pvar), state, &prm->value, &prm->isnull);
+ }
+
+ 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.resvalue = resv;
+ scratch.resnull = resnull;
+ scratch.d.subplan.sstate = sstate;
+
+ ExprEvalPushStep(state, &scratch);
+}
diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
index 4d288bc8d41..f62bb28140f 100644
--- a/src/backend/executor/execProcnode.c
+++ b/src/backend/executor/execProcnode.c
@@ -393,6 +393,10 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
/*
* Initialize any initPlans present in this node. The planner put them in
* a separate list for us.
+ *
+ * The defining characteristic of initplans is that they don't have
+ * arguments, so we don't need to evaluate them (in contrast to
+ * ExecInitSubPlanExpr()).
*/
subps = NIL;
foreach(l, node->initPlan)
@@ -401,6 +405,7 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
SubPlanState *sstate;
Assert(IsA(subplan, SubPlan));
+ Assert(subplan->args == NIL);
sstate = ExecInitSubPlan(subplan, result);
subps = lappend(subps, sstate);
}
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index c136f75ac24..3458ac007cd 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -107,7 +107,7 @@ ExecHashSubPlan(SubPlanState *node,
TupleTableSlot *slot;
/* Shouldn't have any direct correlation Vars */
- if (subplan->parParam != NIL || node->args != NIL)
+ if (subplan->parParam != NIL || subplan->args != NIL)
elog(ERROR, "hashed subplan with direct correlation not supported");
/*
@@ -231,7 +231,6 @@ ExecScanSubPlan(SubPlanState *node,
TupleTableSlot *slot;
Datum result;
bool found = false; /* true if got at least one subplan tuple */
- ListCell *pvar;
ListCell *l;
ArrayBuildStateAny *astate = NULL;
@@ -248,26 +247,20 @@ ExecScanSubPlan(SubPlanState *node,
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.)
+ * We rely on the caller to evaluate plan correlation values, if
+ * necessary. However we still need to record the fact that the values
+ * (might have) changed, otherwise the ExecReScan() below won't know that
+ * nodes need to be rescanned.
*/
- Assert(list_length(subplan->parParam) == list_length(node->args));
-
- forboth(l, subplan->parParam, pvar, node->args)
+ Assert(list_length(subplan->parParam) == list_length(subplan->args));
+ foreach(l, subplan->parParam)
{
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);
}
- /*
- * Now that we've set up its parameters, we can reset the subplan.
- */
+ /* with that done, we can reset the subplan */
ExecReScan(planstate);
/*
@@ -817,6 +810,10 @@ slotNoNulls(TupleTableSlot *slot)
* as well as regular SubPlans. Note that we don't link the SubPlan into
* the parent's subPlan list, because that shouldn't happen for InitPlans.
* Instead, ExecInitExpr() does that one part.
+ *
+ * We also rely on ExecInitExpr(), more precisely ExecInitSubPlanExpr(), to
+ * evaluate input parameters, as that allows them to be evaluated as part of
+ * the expression referencing the SubPlan.
* ----------------------------------------------------------------
*/
SubPlanState *
@@ -844,7 +841,6 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
/* Initialize subexpressions */
sstate->testexpr = ExecInitExpr((Expr *) subplan->testexpr, parent);
- sstate->args = ExecInitExprList(subplan->args, parent);
/*
* initialize my state
@@ -1107,7 +1103,7 @@ 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)
+ if (subplan->parParam || subplan->args)
elog(ERROR, "correlated subplans should not be executed via ExecSetParamPlan");
/*
--
2.38.0
Andres Freund <andres@anarazel.de> writes:
Around
/messages/by-id/20230224015417.75yimxbksejpffh3@awork3.anarazel.de
I suggested that we should evaluate the arguments of correlated SubPlans as
part of the expression referencing the subplan.
Here's a patch for that.
I looked through this, and there is one point that is making me really
uncomfortable. This bit is assuming that we can bind the address of
the es_param_exec_vals array right into the compiled expression:
+ ParamExecData *prm = &estate->es_param_exec_vals[paramid];
+
+ ExecInitExprRec(lfirst(pvar), state, &prm->value, &prm->isnull);Even if that works today, it'd kill the ability to use the compiled
expression across more than one executor instance, which seems like
a pretty high price. Also, I think it probably fails already in
EvalPlanQual contexts, because EvalPlanQualStart allocates a separate
es_param_exec_vals array for EPQ execution.
I think we'd be better off inventing an EEOP_SET_PARAM_EXEC step type
that is essentially the inverse of EEOP_PARAM_EXEC/ExecEvalParamExec,
and then evaluating each parameter value into the expression's
scratch Datum/isnull fields and emitting SET_PARAM_EXEC to copy those
to the correct ParamExecData slot.
regards, tom lane
Hi,
On 2023-03-02 14:33:35 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
Around
/messages/by-id/20230224015417.75yimxbksejpffh3@awork3.anarazel.de
I suggested that we should evaluate the arguments of correlated SubPlans as
part of the expression referencing the subplan.Here's a patch for that.
I looked through this, and there is one point that is making me really
uncomfortable. This bit is assuming that we can bind the address of
the es_param_exec_vals array right into the compiled expression:+ ParamExecData *prm = &estate->es_param_exec_vals[paramid]; + + ExecInitExprRec(lfirst(pvar), state, &prm->value, &prm->isnull);Even if that works today, it'd kill the ability to use the compiled
expression across more than one executor instance, which seems like
a pretty high price. Also, I think it probably fails already in
EvalPlanQual contexts, because EvalPlanQualStart allocates a separate
es_param_exec_vals array for EPQ execution.
Yea, I wasn't super comfortable with that either. I concluded it's ok
because we already cache pointers to the array inside each ExprContext.
I think we'd be better off inventing an EEOP_SET_PARAM_EXEC step type
that is essentially the inverse of EEOP_PARAM_EXEC/ExecEvalParamExec,
and then evaluating each parameter value into the expression's
scratch Datum/isnull fields and emitting SET_PARAM_EXEC to copy those
to the correct ParamExecData slot.
Agreed, that'd make sense. If we can build the infrastructure to figure
out what param to use, that'd also provide a nice basis for using params
for CaseTest etc.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2023-03-02 14:33:35 -0500, Tom Lane wrote:
I looked through this, and there is one point that is making me really
uncomfortable. This bit is assuming that we can bind the address of
the es_param_exec_vals array right into the compiled expression:
Yea, I wasn't super comfortable with that either. I concluded it's ok
because we already cache pointers to the array inside each ExprContext.
ExprContext, sure, but compiled expressions? Considering what it
costs to JIT those, I think we ought to be trying to make them
fairly long-lived.
regards, tom lane
Hi,
On 2023-03-02 15:10:31 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2023-03-02 14:33:35 -0500, Tom Lane wrote:
I looked through this, and there is one point that is making me really
uncomfortable. This bit is assuming that we can bind the address of
the es_param_exec_vals array right into the compiled expression:Yea, I wasn't super comfortable with that either. I concluded it's ok
because we already cache pointers to the array inside each ExprContext.ExprContext, sure, but compiled expressions? Considering what it
costs to JIT those, I think we ought to be trying to make them
fairly long-lived.
I'm not opposed to EXPR_PARAM_SET, to be clear. I'll send an updated
version later. I was just thinking about the correctness in the current
world.
I think it's not just JIT that could benefit, fwiw. I think making
expressions longer lived could also help plpgsql tremendously, for
example.
Greetings,
Andres Freund
Hi,
On 2023-03-02 13:00:31 -0800, Andres Freund wrote:
I'm not opposed to EXPR_PARAM_SET, to be clear. I'll send an updated
version later. I was just thinking about the correctness in the current
world.
Attached.
I named the set EEOP_PARAM_SET EEOP_PARAM_EXEC_SET or such, because I
was wondering if there cases it could also be useful in conjunction with
PARAM_EXTERN, and because nothing really depends on the kind of param.
Greetings,
Andres
Attachments:
v2-0001-WIP-Evaluate-arguments-of-correlated-SubPlans-in-.patchtext/x-diff; charset=us-asciiDownload
From f63915ab00d55b5a67d8b6d05863fe69ea4252b5 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sat, 25 Feb 2023 13:39:19 -0800
Subject: [PATCH v2] WIP: Evaluate arguments of correlated SubPlans in the
referencing ExprState
---
src/include/executor/execExpr.h | 6 +-
src/include/nodes/execnodes.h | 1 -
src/backend/executor/execExpr.c | 93 +++++++++++++++++----------
src/backend/executor/execExprInterp.c | 22 +++++++
src/backend/executor/execProcnode.c | 5 ++
src/backend/executor/nodeSubplan.c | 30 ++++-----
src/backend/jit/llvm/llvmjit_expr.c | 6 ++
src/backend/jit/llvm/llvmjit_types.c | 1 +
8 files changed, 112 insertions(+), 52 deletions(-)
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index 06c3adc0a19..ca2b7306cd0 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -158,6 +158,8 @@ typedef enum ExprEvalOp
EEOP_PARAM_EXEC,
EEOP_PARAM_EXTERN,
EEOP_PARAM_CALLBACK,
+ /* set PARAM_EXEC value */
+ EEOP_PARAM_SET,
/* return CaseTestExpr value */
EEOP_CASE_TESTVAL,
@@ -374,7 +376,7 @@ typedef struct ExprEvalStep
ExprEvalRowtypeCache rowcache;
} nulltest_row;
- /* for EEOP_PARAM_EXEC/EXTERN */
+ /* for EEOP_PARAM_EXEC/EXTERN and EEOP_PARAM_SET */
struct
{
int paramid; /* numeric ID for parameter */
@@ -738,6 +740,8 @@ extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
+extern void ExecEvalParamSet(ExprState *state, ExprEvalStep *op,
+ ExprContext *econtext);
extern void ExecEvalCurrentOfExpr(ExprState *state, ExprEvalStep *op);
extern void ExecEvalNextValueExpr(ExprState *state, ExprEvalStep *op);
extern void ExecEvalRowNull(ExprState *state, ExprEvalStep *op,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index e7eb1e666ff..16e95e4cb48 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -947,7 +947,6 @@ typedef struct SubPlanState
struct PlanState *planstate; /* subselect plan's state tree */
struct PlanState *parent; /* parent plan node's state tree */
ExprState *testexpr; /* state of combining expression */
- List *args; /* states of argument expression(s) */
HeapTuple curTuple; /* copy of most recent tuple from subplan */
Datum curArray; /* most recent array from ARRAY() subplan */
/* these are used when hashing the subselect's output: */
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index c61f23c6c18..002f2a0091f 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -87,6 +87,9 @@ static void ExecBuildAggTransCall(ExprState *state, AggState *aggstate,
FunctionCallInfo fcinfo, AggStatePerTrans pertrans,
int transno, int setno, int setoff, bool ishash,
bool nullcheck);
+static void ExecInitSubPlanExpr(SubPlan *subplan,
+ ExprState *state,
+ Datum *resv, bool *resnull);
/*
@@ -1388,7 +1391,6 @@ ExecInitExprRec(Expr *node, ExprState *state,
case T_SubPlan:
{
SubPlan *subplan = (SubPlan *) node;
- SubPlanState *sstate;
/*
* Real execution of a MULTIEXPR SubPlan has already been
@@ -1405,19 +1407,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
break;
}
- 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;
-
- ExprEvalPushStep(state, &scratch);
+ ExecInitSubPlanExpr(subplan, state, resv, resnull);
break;
}
@@ -2618,29 +2608,12 @@ ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info)
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);
+ ExecInitSubPlanExpr(subplan, state,
+ &state->resvalue, &state->resnull);
}
}
@@ -4040,3 +4013,57 @@ ExecBuildParamSetEqual(TupleDesc desc,
return state;
}
+
+static void
+ExecInitSubPlanExpr(SubPlan *subplan,
+ ExprState *state,
+ Datum *resv, bool *resnull)
+{
+ ExprEvalStep scratch = {0};
+ SubPlanState *sstate;
+ ListCell *pvar;
+ ListCell *l;
+
+ if (!state->parent)
+ elog(ERROR, "SubPlan found with no parent plan");
+
+ /*
+ * Generate steps to evaluate input arguments for the subplan.
+ *
+ * We evaluate the argument expressions into ExprState's resvalue/resnull,
+ * and then use PARAM_SET to update the parameter. We do that, instead of
+ * evaluating directly into the param, to avoid depending on the pointer
+ * value remaining stable / being included in the generated expression. No
+ * danger of conflicts with other uses of resvalue/resnull as storing and
+ * using the value always is in subsequent steps.
+ *
+ * Any calculation we have to do can be done in the parent econtext, since
+ * the Param values don't need to have per-query lifetime.
+ */
+ forboth(l, subplan->parParam, pvar, subplan->args)
+ {
+ int paramid = lfirst_int(l);
+
+ ExecInitExprRec(lfirst(pvar), state,
+ &state->resvalue, &state->resnull);
+
+ scratch.opcode = EEOP_PARAM_SET;
+ scratch.d.param.paramid = paramid;
+ /* type isn't needed, but an old value could be confusing */
+ scratch.d.param.paramtype = InvalidOid;
+ ExprEvalPushStep(state, &scratch);
+ }
+
+ 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.resvalue = resv;
+ scratch.resnull = resnull;
+ scratch.d.subplan.sstate = sstate;
+
+ ExprEvalPushStep(state, &scratch);
+}
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 19351fe34bf..3cab8a5cdae 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -446,6 +446,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
&&CASE_EEOP_PARAM_EXEC,
&&CASE_EEOP_PARAM_EXTERN,
&&CASE_EEOP_PARAM_CALLBACK,
+ &&CASE_EEOP_PARAM_SET,
&&CASE_EEOP_CASE_TESTVAL,
&&CASE_EEOP_MAKE_READONLY,
&&CASE_EEOP_IOCOERCE,
@@ -1081,6 +1082,13 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
EEO_NEXT();
}
+ EEO_CASE(EEOP_PARAM_SET)
+ {
+ /* out of line, unlikely to matter performancewise */
+ ExecEvalParamSet(state, op, econtext);
+ EEO_NEXT();
+ }
+
EEO_CASE(EEOP_CASE_TESTVAL)
{
/*
@@ -2477,6 +2485,20 @@ ExecEvalParamExtern(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
errmsg("no value found for parameter %d", paramId)));
}
+/*
+ * Set value of a param (currently always PARAM_EXEC) from
+ * state->res{value,null}.
+ */
+void
+ExecEvalParamSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
+{
+ ParamExecData *prm;
+
+ prm = &(econtext->ecxt_param_exec_vals[op->d.param.paramid]);
+ prm->value = state->resvalue;
+ prm->isnull = state->resnull;
+}
+
/*
* Raise error if a CURRENT OF expression is evaluated.
*
diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
index 4d288bc8d41..f62bb28140f 100644
--- a/src/backend/executor/execProcnode.c
+++ b/src/backend/executor/execProcnode.c
@@ -393,6 +393,10 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
/*
* Initialize any initPlans present in this node. The planner put them in
* a separate list for us.
+ *
+ * The defining characteristic of initplans is that they don't have
+ * arguments, so we don't need to evaluate them (in contrast to
+ * ExecInitSubPlanExpr()).
*/
subps = NIL;
foreach(l, node->initPlan)
@@ -401,6 +405,7 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
SubPlanState *sstate;
Assert(IsA(subplan, SubPlan));
+ Assert(subplan->args == NIL);
sstate = ExecInitSubPlan(subplan, result);
subps = lappend(subps, sstate);
}
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index c136f75ac24..3458ac007cd 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -107,7 +107,7 @@ ExecHashSubPlan(SubPlanState *node,
TupleTableSlot *slot;
/* Shouldn't have any direct correlation Vars */
- if (subplan->parParam != NIL || node->args != NIL)
+ if (subplan->parParam != NIL || subplan->args != NIL)
elog(ERROR, "hashed subplan with direct correlation not supported");
/*
@@ -231,7 +231,6 @@ ExecScanSubPlan(SubPlanState *node,
TupleTableSlot *slot;
Datum result;
bool found = false; /* true if got at least one subplan tuple */
- ListCell *pvar;
ListCell *l;
ArrayBuildStateAny *astate = NULL;
@@ -248,26 +247,20 @@ ExecScanSubPlan(SubPlanState *node,
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.)
+ * We rely on the caller to evaluate plan correlation values, if
+ * necessary. However we still need to record the fact that the values
+ * (might have) changed, otherwise the ExecReScan() below won't know that
+ * nodes need to be rescanned.
*/
- Assert(list_length(subplan->parParam) == list_length(node->args));
-
- forboth(l, subplan->parParam, pvar, node->args)
+ Assert(list_length(subplan->parParam) == list_length(subplan->args));
+ foreach(l, subplan->parParam)
{
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);
}
- /*
- * Now that we've set up its parameters, we can reset the subplan.
- */
+ /* with that done, we can reset the subplan */
ExecReScan(planstate);
/*
@@ -817,6 +810,10 @@ slotNoNulls(TupleTableSlot *slot)
* as well as regular SubPlans. Note that we don't link the SubPlan into
* the parent's subPlan list, because that shouldn't happen for InitPlans.
* Instead, ExecInitExpr() does that one part.
+ *
+ * We also rely on ExecInitExpr(), more precisely ExecInitSubPlanExpr(), to
+ * evaluate input parameters, as that allows them to be evaluated as part of
+ * the expression referencing the SubPlan.
* ----------------------------------------------------------------
*/
SubPlanState *
@@ -844,7 +841,6 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
/* Initialize subexpressions */
sstate->testexpr = ExecInitExpr((Expr *) subplan->testexpr, parent);
- sstate->args = ExecInitExprList(subplan->args, parent);
/*
* initialize my state
@@ -1107,7 +1103,7 @@ 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)
+ if (subplan->parParam || subplan->args)
elog(ERROR, "correlated subplans should not be executed via ExecSetParamPlan");
/*
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 1c722c79552..812f8758743 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1094,6 +1094,12 @@ llvm_compile_expr(ExprState *state)
break;
}
+ case EEOP_PARAM_SET:
+ build_EvalXFunc(b, mod, "ExecEvalParamSet",
+ v_state, op, v_econtext);
+ LLVMBuildBr(b, opblocks[opno + 1]);
+ break;
+
case EEOP_SBSREF_SUBSCRIPTS:
{
int jumpdone = op->d.sbsref_subscript.jumpdone;
diff --git a/src/backend/jit/llvm/llvmjit_types.c b/src/backend/jit/llvm/llvmjit_types.c
index 876fb640294..0b1df3afe00 100644
--- a/src/backend/jit/llvm/llvmjit_types.c
+++ b/src/backend/jit/llvm/llvmjit_types.c
@@ -123,6 +123,7 @@ void *referenced_functions[] =
ExecEvalNextValueExpr,
ExecEvalParamExec,
ExecEvalParamExtern,
+ ExecEvalParamSet,
ExecEvalRow,
ExecEvalRowNotNull,
ExecEvalRowNull,
--
2.37.1.188.g71a8fab31b
Andres Freund <andres@anarazel.de> writes:
On 2023-03-02 13:00:31 -0800, Andres Freund wrote:
I'm not opposed to EXPR_PARAM_SET, to be clear. I'll send an updated
version later. I was just thinking about the correctness in the current
world.
Attached.
I've looked through this, and it looks basically OK so I marked it RfC.
I do have a few nitpicks that you might or might not choose to adopt:
It'd be good to have a header comment for ExecInitExprRec documenting
the arguments, particularly that resv/resnull are where to put the
subplan's eventual result.
You could avoid having to assume ExprState's resvalue/resnull being
safe to use by instead using the target resv/resnull. This would
require putting those into the EEOP_PARAM_SET step so that
ExecEvalParamSet knows where to fetch from, so maybe it's not an
improvement, but perhaps worth considering.
+ /* type isn't needed, but an old value could be confusing */
+ scratch.d.param.paramtype = InvalidOid;
I'd just store the param's type, rather than justifying why you didn't.
It's cheap enough and even less confusing.I think that ExecEvalParamSet should either set prm->execPlan to NULL,
or maybe better Assert that it is already NULL.
It's a bit weird to keep this in ExecScanSubPlan, when the code there
no longer depends on it:
+ Assert(list_length(subplan->parParam) == list_length(subplan->args));
I'd put that before the forboth() in ExecInitSubPlanExpr instead,
where it does matter.
regards, tom lane
Hi,
On 2023-03-03 15:09:18 -0500, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2023-03-02 13:00:31 -0800, Andres Freund wrote:
I'm not opposed to EXPR_PARAM_SET, to be clear. I'll send an updated
version later. I was just thinking about the correctness in the current
world.Attached.
I've looked through this, and it looks basically OK so I marked it RfC.
Thanks!
I do have a few nitpicks that you might or might not choose to adopt:
It'd be good to have a header comment for ExecInitExprRec documenting
the arguments, particularly that resv/resnull are where to put the
subplan's eventual result.
Did you mean ExecInitSubPlanExpr()?
You could avoid having to assume ExprState's resvalue/resnull being
safe to use by instead using the target resv/resnull. This would
require putting those into the EEOP_PARAM_SET step so that
ExecEvalParamSet knows where to fetch from, so maybe it's not an
improvement, but perhaps worth considering.
I think that'd be a bit worse - we'd have more pointers that can't be handled
in a generic way in JIT.
I think that ExecEvalParamSet should either set prm->execPlan to NULL,
or maybe better Assert that it is already NULL.
Agreed.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2023-03-03 15:09:18 -0500, Tom Lane wrote:
It'd be good to have a header comment for ExecInitExprRec documenting
the arguments, particularly that resv/resnull are where to put the
subplan's eventual result.
Did you mean ExecInitSubPlanExpr()?
Right, copy-and-pasteo, sorry.
You could avoid having to assume ExprState's resvalue/resnull being
safe to use by instead using the target resv/resnull. This would
require putting those into the EEOP_PARAM_SET step so that
ExecEvalParamSet knows where to fetch from, so maybe it's not an
improvement, but perhaps worth considering.
I think that'd be a bit worse - we'd have more pointers that can't be handled
in a generic way in JIT.
OK.
regards, tom lane
Is this patch still being worked on?
Show quoted text
On 07.03.23 01:51, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
On 2023-03-03 15:09:18 -0500, Tom Lane wrote:
It'd be good to have a header comment for ExecInitExprRec documenting
the arguments, particularly that resv/resnull are where to put the
subplan's eventual result.Did you mean ExecInitSubPlanExpr()?
Right, copy-and-pasteo, sorry.
You could avoid having to assume ExprState's resvalue/resnull being
safe to use by instead using the target resv/resnull. This would
require putting those into the EEOP_PARAM_SET step so that
ExecEvalParamSet knows where to fetch from, so maybe it's not an
improvement, but perhaps worth considering.I think that'd be a bit worse - we'd have more pointers that can't be handled
in a generic way in JIT.OK.
regards, tom lane
Peter Eisentraut <peter@eisentraut.org> writes:
Is this patch still being worked on?
I thought Andres simply hadn't gotten back to it yet.
It still seems like a worthwhile improvement.
regards, tom lane
Hi,
On 2023-10-01 14:53:23 -0400, Tom Lane wrote:
Peter Eisentraut <peter@eisentraut.org> writes:
Is this patch still being worked on?
I thought Andres simply hadn't gotten back to it yet.
It still seems like a worthwhile improvement.
Indeed - I do plan to commit it. I haven't quite shifted into v17 mode yet...
Greetings,
Andres Freund
Hi!
I looked through your patch and noticed that it was not applied to the
current version of the master. I rebased it and attached a version. I
didn't see any problems and, honestly, no big changes were needed, all
regression tests were passed.
I think it's better to add a test, but to be honest, I haven't been able
to come up with something yet.
--
Regards,
Alena Rybakina
Attachments:
v2-0001-WIP-Evaluate-arguments-of-correlated-SubPlans-in-the.patchtext/x-patch; charset=UTF-8; name=v2-0001-WIP-Evaluate-arguments-of-correlated-SubPlans-in-the.patchDownload
From f7a8ca7f3263fa5f82056f90231cf937133622c9 Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Mon, 23 Oct 2023 22:54:04 +0300
Subject: [PATCH] WIP: Evaluate arguments of correlated SubPlans in the
referencing ExprState
---
src/backend/executor/execExpr.c | 93 +++++++++++++++++----------
src/backend/executor/execExprInterp.c | 23 +++++++
src/backend/executor/execProcnode.c | 5 ++
src/backend/executor/nodeSubplan.c | 30 ++++-----
src/backend/jit/llvm/llvmjit_expr.c | 6 ++
src/backend/jit/llvm/llvmjit_types.c | 1 +
src/include/executor/execExpr.h | 6 +-
src/include/nodes/execnodes.h | 1 -
8 files changed, 113 insertions(+), 52 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 2c62b0c9c84..14cd56a2db3 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -88,6 +88,9 @@ static void ExecBuildAggTransCall(ExprState *state, AggState *aggstate,
FunctionCallInfo fcinfo, AggStatePerTrans pertrans,
int transno, int setno, int setoff, bool ishash,
bool nullcheck);
+static void ExecInitSubPlanExpr(SubPlan *subplan,
+ ExprState *state,
+ Datum *resv, bool *resnull);
/*
@@ -1389,7 +1392,6 @@ ExecInitExprRec(Expr *node, ExprState *state,
case T_SubPlan:
{
SubPlan *subplan = (SubPlan *) node;
- SubPlanState *sstate;
/*
* Real execution of a MULTIEXPR SubPlan has already been
@@ -1406,19 +1408,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
break;
}
- 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;
-
- ExprEvalPushStep(state, &scratch);
+ ExecInitSubPlanExpr(subplan, state, resv, resnull);
break;
}
@@ -2750,29 +2740,12 @@ ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info)
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);
+ ExecInitSubPlanExpr(subplan, state,
+ &state->resvalue, &state->resnull);
}
}
@@ -4178,3 +4151,57 @@ ExecBuildParamSetEqual(TupleDesc desc,
return state;
}
+
+static void
+ExecInitSubPlanExpr(SubPlan *subplan,
+ ExprState *state,
+ Datum *resv, bool *resnull)
+{
+ ExprEvalStep scratch = {0};
+ SubPlanState *sstate;
+ ListCell *pvar;
+ ListCell *l;
+
+ if (!state->parent)
+ elog(ERROR, "SubPlan found with no parent plan");
+
+ /*
+ * Generate steps to evaluate input arguments for the subplan.
+ *
+ * We evaluate the argument expressions into ExprState's resvalue/resnull,
+ * and then use PARAM_SET to update the parameter. We do that, instead of
+ * evaluating directly into the param, to avoid depending on the pointer
+ * value remaining stable / being included in the generated expression. No
+ * danger of conflicts with other uses of resvalue/resnull as storing and
+ * using the value always is in subsequent steps.
+ *
+ * Any calculation we have to do can be done in the parent econtext, since
+ * the Param values don't need to have per-query lifetime.
+ */
+ forboth(l, subplan->parParam, pvar, subplan->args)
+ {
+ int paramid = lfirst_int(l);
+
+ ExecInitExprRec(lfirst(pvar), state,
+ &state->resvalue, &state->resnull);
+
+ scratch.opcode = EEOP_PARAM_SET;
+ scratch.d.param.paramid = paramid;
+ /* type isn't needed, but an old value could be confusing */
+ scratch.d.param.paramtype = InvalidOid;
+ ExprEvalPushStep(state, &scratch);
+ }
+
+ 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.resvalue = resv;
+ scratch.resnull = resnull;
+ scratch.d.subplan.sstate = sstate;
+
+ ExprEvalPushStep(state, &scratch);
+}
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 24c2b60c62a..e3981df3de8 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -449,6 +449,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
&&CASE_EEOP_PARAM_EXEC,
&&CASE_EEOP_PARAM_EXTERN,
&&CASE_EEOP_PARAM_CALLBACK,
+ &&CASE_EEOP_PARAM_SET,
&&CASE_EEOP_CASE_TESTVAL,
&&CASE_EEOP_MAKE_READONLY,
&&CASE_EEOP_IOCOERCE,
@@ -1087,6 +1088,13 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
EEO_NEXT();
}
+ EEO_CASE(EEOP_PARAM_SET)
+ {
+ /* out of line, unlikely to matter performancewise */
+ ExecEvalParamSet(state, op, econtext);
+ EEO_NEXT();
+ }
+
EEO_CASE(EEOP_CASE_TESTVAL)
{
/*
@@ -2510,6 +2518,21 @@ ExecEvalParamExtern(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
errmsg("no value found for parameter %d", paramId)));
}
+/*
+ * Set value of a param (currently always PARAM_EXEC) from
+ * state->res{value,null}.
+ */
+void
+ExecEvalParamSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
+{
+ ParamExecData *prm;
+
+ prm = &(econtext->ecxt_param_exec_vals[op->d.param.paramid]);
+ prm->value = state->resvalue;
+ prm->isnull = state->resnull;
+}
+
+
/*
* Evaluate a SQLValueFunction expression.
*/
diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
index b4b5c562c0f..13ac3e600e6 100644
--- a/src/backend/executor/execProcnode.c
+++ b/src/backend/executor/execProcnode.c
@@ -393,6 +393,10 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
/*
* Initialize any initPlans present in this node. The planner put them in
* a separate list for us.
+ *
+ * The defining characteristic of initplans is that they don't have
+ * arguments, so we don't need to evaluate them (in contrast to
+ * ExecInitSubPlanExpr()).
*/
subps = NIL;
foreach(l, node->initPlan)
@@ -401,6 +405,7 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
SubPlanState *sstate;
Assert(IsA(subplan, SubPlan));
+ Assert(subplan->args == NIL);
sstate = ExecInitSubPlan(subplan, result);
subps = lappend(subps, sstate);
}
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index c136f75ac24..3458ac007cd 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -107,7 +107,7 @@ ExecHashSubPlan(SubPlanState *node,
TupleTableSlot *slot;
/* Shouldn't have any direct correlation Vars */
- if (subplan->parParam != NIL || node->args != NIL)
+ if (subplan->parParam != NIL || subplan->args != NIL)
elog(ERROR, "hashed subplan with direct correlation not supported");
/*
@@ -231,7 +231,6 @@ ExecScanSubPlan(SubPlanState *node,
TupleTableSlot *slot;
Datum result;
bool found = false; /* true if got at least one subplan tuple */
- ListCell *pvar;
ListCell *l;
ArrayBuildStateAny *astate = NULL;
@@ -248,26 +247,20 @@ ExecScanSubPlan(SubPlanState *node,
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.)
+ * We rely on the caller to evaluate plan correlation values, if
+ * necessary. However we still need to record the fact that the values
+ * (might have) changed, otherwise the ExecReScan() below won't know that
+ * nodes need to be rescanned.
*/
- Assert(list_length(subplan->parParam) == list_length(node->args));
-
- forboth(l, subplan->parParam, pvar, node->args)
+ Assert(list_length(subplan->parParam) == list_length(subplan->args));
+ foreach(l, subplan->parParam)
{
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);
}
- /*
- * Now that we've set up its parameters, we can reset the subplan.
- */
+ /* with that done, we can reset the subplan */
ExecReScan(planstate);
/*
@@ -817,6 +810,10 @@ slotNoNulls(TupleTableSlot *slot)
* as well as regular SubPlans. Note that we don't link the SubPlan into
* the parent's subPlan list, because that shouldn't happen for InitPlans.
* Instead, ExecInitExpr() does that one part.
+ *
+ * We also rely on ExecInitExpr(), more precisely ExecInitSubPlanExpr(), to
+ * evaluate input parameters, as that allows them to be evaluated as part of
+ * the expression referencing the SubPlan.
* ----------------------------------------------------------------
*/
SubPlanState *
@@ -844,7 +841,6 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
/* Initialize subexpressions */
sstate->testexpr = ExecInitExpr((Expr *) subplan->testexpr, parent);
- sstate->args = ExecInitExprList(subplan->args, parent);
/*
* initialize my state
@@ -1107,7 +1103,7 @@ 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)
+ if (subplan->parParam || subplan->args)
elog(ERROR, "correlated subplans should not be executed via ExecSetParamPlan");
/*
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index a3a0876bfff..0b2d4d9057c 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1145,6 +1145,12 @@ llvm_compile_expr(ExprState *state)
break;
}
+ case EEOP_PARAM_SET:
+ build_EvalXFunc(b, mod, "ExecEvalParamSet",
+ v_state, op, v_econtext);
+ LLVMBuildBr(b, opblocks[opno + 1]);
+ break;
+
case EEOP_SBSREF_SUBSCRIPTS:
{
int jumpdone = op->d.sbsref_subscript.jumpdone;
diff --git a/src/backend/jit/llvm/llvmjit_types.c b/src/backend/jit/llvm/llvmjit_types.c
index 791902ff1f3..78bf99c1cbc 100644
--- a/src/backend/jit/llvm/llvmjit_types.c
+++ b/src/backend/jit/llvm/llvmjit_types.c
@@ -159,6 +159,7 @@ void *referenced_functions[] =
ExecEvalNextValueExpr,
ExecEvalParamExec,
ExecEvalParamExtern,
+ ExecEvalParamSet,
ExecEvalRow,
ExecEvalRowNotNull,
ExecEvalRowNull,
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index 048573c2bcb..91b67454eb4 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -159,6 +159,8 @@ typedef enum ExprEvalOp
EEOP_PARAM_EXEC,
EEOP_PARAM_EXTERN,
EEOP_PARAM_CALLBACK,
+ /* set PARAM_EXEC value */
+ EEOP_PARAM_SET,
/* return CaseTestExpr value */
EEOP_CASE_TESTVAL,
@@ -378,7 +380,7 @@ typedef struct ExprEvalStep
ExprEvalRowtypeCache rowcache;
} nulltest_row;
- /* for EEOP_PARAM_EXEC/EXTERN */
+ /* for EEOP_PARAM_EXEC/EXTERN and EEOP_PARAM_SET */
struct
{
int paramid; /* numeric ID for parameter */
@@ -776,6 +778,8 @@ extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
+extern void ExecEvalParamSet(ExprState *state, ExprEvalStep *op,
+ ExprContext *econtext);
extern void ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op);
extern void ExecEvalCurrentOfExpr(ExprState *state, ExprEvalStep *op);
extern void ExecEvalNextValueExpr(ExprState *state, ExprEvalStep *op);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 108d69ba287..88a128a5514 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -952,7 +952,6 @@ typedef struct SubPlanState
struct PlanState *planstate; /* subselect plan's state tree */
struct PlanState *parent; /* parent plan node's state tree */
ExprState *testexpr; /* state of combining expression */
- List *args; /* states of argument expression(s) */
HeapTuple curTuple; /* copy of most recent tuple from subplan */
Datum curArray; /* most recent array from ARRAY() subplan */
/* these are used when hashing the subselect's output: */
--
2.34.1
On Tue, Oct 10, 2023 at 10:00 AM Andres Freund <andres@anarazel.de> wrote:
Hi,
On 2023-10-01 14:53:23 -0400, Tom Lane wrote:
Peter Eisentraut <peter@eisentraut.org> writes:
Is this patch still being worked on?
I thought Andres simply hadn't gotten back to it yet.
It still seems like a worthwhile improvement.Indeed - I do plan to commit it. I haven't quite shifted into v17 mode yet...
Any shift yet? ;-)
2024-01 Commitfest.
Hi, This patch has a CF status of "Ready for Committer", but it is
currently failing some CFbot tests [1]https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4209. Please have a look and post an
updated version..
======
[1]: https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4209
Kind Regards,
Peter Smith.
Hi,
On 2024-01-22 10:30:22 +1100, Peter Smith wrote:
2024-01 Commitfest.
Hi, This patch has a CF status of "Ready for Committer", but it is
currently failing some CFbot tests [1]. Please have a look and post an
updated version..
I think this failure is independent of this patch - by coincidence I just
sent an email about the issue
/messages/by-id/20240122204117.swton324xcoodnyi@awork3.anarazel.de
a few minutes ago.
Greetings,
Andres Freund
On Tue, 24 Oct 2023 at 01:47, Alena Rybakina <lena.ribackina@yandex.ru> wrote:
Hi!
I looked through your patch and noticed that it was not applied to the current version of the master. I rebased it and attached a version. I didn't see any problems and, honestly, no big changes were needed, all regression tests were passed.
I think it's better to add a test, but to be honest, I haven't been able to come up with something yet.
The patch does not apply anymore as in CFBot at [1]http://cfbot.cputube.org/patch_46_4209.log:
=== Applying patches on top of PostgreSQL commit ID
7014c9a4bba2d1b67d60687afb5b2091c1d07f73 ===
=== applying patch
./v2-0001-WIP-Evaluate-arguments-of-correlated-SubPlans-in-the.patch
....
patching file src/include/executor/execExpr.h
Hunk #1 succeeded at 160 (offset 1 line).
Hunk #2 succeeded at 382 (offset 2 lines).
Hunk #3 FAILED at 778.
1 out of 3 hunks FAILED -- saving rejects to file
src/include/executor/execExpr.h.rej
patching file src/include/nodes/execnodes.h
Hunk #1 succeeded at 959 (offset 7 lines).
Please have a look and post an updated version.
[1]: http://cfbot.cputube.org/patch_46_4209.log
Regards,
Vignesh
On 26.01.2024 05:37, vignesh C wrote:
On Tue, 24 Oct 2023 at 01:47, Alena Rybakina <lena.ribackina@yandex.ru> wrote:
Hi!
I looked through your patch and noticed that it was not applied to the current version of the master. I rebased it and attached a version. I didn't see any problems and, honestly, no big changes were needed, all regression tests were passed.
I think it's better to add a test, but to be honest, I haven't been able to come up with something yet.
The patch does not apply anymore as in CFBot at [1]:
=== Applying patches on top of PostgreSQL commit ID
7014c9a4bba2d1b67d60687afb5b2091c1d07f73 ===
=== applying patch
./v2-0001-WIP-Evaluate-arguments-of-correlated-SubPlans-in-the.patch
....
patching file src/include/executor/execExpr.h
Hunk #1 succeeded at 160 (offset 1 line).
Hunk #2 succeeded at 382 (offset 2 lines).
Hunk #3 FAILED at 778.
1 out of 3 hunks FAILED -- saving rejects to file
src/include/executor/execExpr.h.rej
patching file src/include/nodes/execnodes.h
Hunk #1 succeeded at 959 (offset 7 lines).Please have a look and post an updated version.
[1] - http://cfbot.cputube.org/patch_46_4209.log
Regards,
Vignesh
Thank you!
I fixed it. The code remains the same.
--
Regards,
Alena Rybakina
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
v3-0001-WIP-Evaluate-arguments-of-correlated-SubPlans-in-the.patchtext/x-patch; charset=UTF-8; name=v3-0001-WIP-Evaluate-arguments-of-correlated-SubPlans-in-the.patchDownload
From bf40b14c0cb63f47280299fd3f76a1711db6aada Mon Sep 17 00:00:00 2001
From: Alena Rybakina <a.rybakina@postgrespro.ru>
Date: Sun, 28 Jan 2024 11:58:44 +0300
Subject: [PATCH] WIP: Evaluate arguments of correlated SubPlans in the
referencing ExprState.
---
src/backend/executor/execExpr.c | 93 +++++++++++++++++----------
src/backend/executor/execExprInterp.c | 22 +++++++
src/backend/executor/execProcnode.c | 5 ++
src/backend/executor/nodeSubplan.c | 30 ++++-----
src/backend/jit/llvm/llvmjit_expr.c | 6 ++
src/backend/jit/llvm/llvmjit_types.c | 1 +
src/include/executor/execExpr.h | 6 +-
src/include/nodes/execnodes.h | 1 -
8 files changed, 112 insertions(+), 52 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 3181b1136a2..d2e539e7b28 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -88,6 +88,9 @@ static void ExecBuildAggTransCall(ExprState *state, AggState *aggstate,
FunctionCallInfo fcinfo, AggStatePerTrans pertrans,
int transno, int setno, int setoff, bool ishash,
bool nullcheck);
+static void ExecInitSubPlanExpr(SubPlan *subplan,
+ ExprState *state,
+ Datum *resv, bool *resnull);
/*
@@ -1386,7 +1389,6 @@ ExecInitExprRec(Expr *node, ExprState *state,
case T_SubPlan:
{
SubPlan *subplan = (SubPlan *) node;
- SubPlanState *sstate;
/*
* Real execution of a MULTIEXPR SubPlan has already been
@@ -1403,19 +1405,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
break;
}
- 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;
-
- ExprEvalPushStep(state, &scratch);
+ ExecInitSubPlanExpr(subplan, state, resv, resnull);
break;
}
@@ -2752,29 +2742,12 @@ ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info)
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);
+ ExecInitSubPlanExpr(subplan, state,
+ &state->resvalue, &state->resnull);
}
}
@@ -4181,3 +4154,57 @@ ExecBuildParamSetEqual(TupleDesc desc,
return state;
}
+
+static void
+ExecInitSubPlanExpr(SubPlan *subplan,
+ ExprState *state,
+ Datum *resv, bool *resnull)
+{
+ ExprEvalStep scratch = {0};
+ SubPlanState *sstate;
+ ListCell *pvar;
+ ListCell *l;
+
+ if (!state->parent)
+ elog(ERROR, "SubPlan found with no parent plan");
+
+ /*
+ * Generate steps to evaluate input arguments for the subplan.
+ *
+ * We evaluate the argument expressions into ExprState's resvalue/resnull,
+ * and then use PARAM_SET to update the parameter. We do that, instead of
+ * evaluating directly into the param, to avoid depending on the pointer
+ * value remaining stable / being included in the generated expression. No
+ * danger of conflicts with other uses of resvalue/resnull as storing and
+ * using the value always is in subsequent steps.
+ *
+ * Any calculation we have to do can be done in the parent econtext, since
+ * the Param values don't need to have per-query lifetime.
+ */
+ forboth(l, subplan->parParam, pvar, subplan->args)
+ {
+ int paramid = lfirst_int(l);
+
+ ExecInitExprRec(lfirst(pvar), state,
+ &state->resvalue, &state->resnull);
+
+ scratch.opcode = EEOP_PARAM_SET;
+ scratch.d.param.paramid = paramid;
+ /* type isn't needed, but an old value could be confusing */
+ scratch.d.param.paramtype = InvalidOid;
+ ExprEvalPushStep(state, &scratch);
+ }
+
+ 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.resvalue = resv;
+ scratch.resnull = resnull;
+ scratch.d.subplan.sstate = sstate;
+
+ ExprEvalPushStep(state, &scratch);
+}
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 3f20f1dd314..edab4ed357b 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -450,6 +450,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
&&CASE_EEOP_PARAM_EXEC,
&&CASE_EEOP_PARAM_EXTERN,
&&CASE_EEOP_PARAM_CALLBACK,
+ &&CASE_EEOP_PARAM_SET,
&&CASE_EEOP_CASE_TESTVAL,
&&CASE_EEOP_MAKE_READONLY,
&&CASE_EEOP_IOCOERCE,
@@ -1089,6 +1090,13 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
EEO_NEXT();
}
+ EEO_CASE(EEOP_PARAM_SET)
+ {
+ /* out of line, unlikely to matter performancewise */
+ ExecEvalParamSet(state, op, econtext);
+ EEO_NEXT();
+ }
+
EEO_CASE(EEOP_CASE_TESTVAL)
{
/*
@@ -2521,6 +2529,20 @@ ExecEvalParamExtern(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
errmsg("no value found for parameter %d", paramId)));
}
+/*
+ * Set value of a param (currently always PARAM_EXEC) from
+ * state->res{value,null}.
+ */
+void
+ExecEvalParamSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
+{
+ ParamExecData *prm;
+
+ prm = &(econtext->ecxt_param_exec_vals[op->d.param.paramid]);
+ prm->value = state->resvalue;
+ prm->isnull = state->resnull;
+}
+
/*
* Evaluate a CoerceViaIO node in soft-error mode.
*
diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
index 6e48062f56a..34f28dfecea 100644
--- a/src/backend/executor/execProcnode.c
+++ b/src/backend/executor/execProcnode.c
@@ -393,6 +393,10 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
/*
* Initialize any initPlans present in this node. The planner put them in
* a separate list for us.
+ *
+ * The defining characteristic of initplans is that they don't have
+ * arguments, so we don't need to evaluate them (in contrast to
+ * ExecInitSubPlanExpr()).
*/
subps = NIL;
foreach(l, node->initPlan)
@@ -401,6 +405,7 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
SubPlanState *sstate;
Assert(IsA(subplan, SubPlan));
+ Assert(subplan->args == NIL);
sstate = ExecInitSubPlan(subplan, result);
subps = lappend(subps, sstate);
}
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 9697b1f396d..2335a828f4f 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -107,7 +107,7 @@ ExecHashSubPlan(SubPlanState *node,
TupleTableSlot *slot;
/* Shouldn't have any direct correlation Vars */
- if (subplan->parParam != NIL || node->args != NIL)
+ if (subplan->parParam != NIL || subplan->args != NIL)
elog(ERROR, "hashed subplan with direct correlation not supported");
/*
@@ -231,7 +231,6 @@ ExecScanSubPlan(SubPlanState *node,
TupleTableSlot *slot;
Datum result;
bool found = false; /* true if got at least one subplan tuple */
- ListCell *pvar;
ListCell *l;
ArrayBuildStateAny *astate = NULL;
@@ -248,26 +247,20 @@ ExecScanSubPlan(SubPlanState *node,
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.)
+ * We rely on the caller to evaluate plan correlation values, if
+ * necessary. However we still need to record the fact that the values
+ * (might have) changed, otherwise the ExecReScan() below won't know that
+ * nodes need to be rescanned.
*/
- Assert(list_length(subplan->parParam) == list_length(node->args));
-
- forboth(l, subplan->parParam, pvar, node->args)
+ Assert(list_length(subplan->parParam) == list_length(subplan->args));
+ foreach(l, subplan->parParam)
{
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);
}
- /*
- * Now that we've set up its parameters, we can reset the subplan.
- */
+ /* with that done, we can reset the subplan */
ExecReScan(planstate);
/*
@@ -817,6 +810,10 @@ slotNoNulls(TupleTableSlot *slot)
* as well as regular SubPlans. Note that we don't link the SubPlan into
* the parent's subPlan list, because that shouldn't happen for InitPlans.
* Instead, ExecInitExpr() does that one part.
+ *
+ * We also rely on ExecInitExpr(), more precisely ExecInitSubPlanExpr(), to
+ * evaluate input parameters, as that allows them to be evaluated as part of
+ * the expression referencing the SubPlan.
* ----------------------------------------------------------------
*/
SubPlanState *
@@ -844,7 +841,6 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
/* Initialize subexpressions */
sstate->testexpr = ExecInitExpr((Expr *) subplan->testexpr, parent);
- sstate->args = ExecInitExprList(subplan->args, parent);
/*
* initialize my state
@@ -1107,7 +1103,7 @@ 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)
+ if (subplan->parParam || subplan->args)
elog(ERROR, "correlated subplans should not be executed via ExecSetParamPlan");
/*
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 0c448422e20..88eee19c50e 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1145,6 +1145,12 @@ llvm_compile_expr(ExprState *state)
break;
}
+ case EEOP_PARAM_SET:
+ build_EvalXFunc(b, mod, "ExecEvalParamSet",
+ v_state, op, v_econtext);
+ LLVMBuildBr(b, opblocks[opno + 1]);
+ break;
+
case EEOP_SBSREF_SUBSCRIPTS:
{
int jumpdone = op->d.sbsref_subscript.jumpdone;
diff --git a/src/backend/jit/llvm/llvmjit_types.c b/src/backend/jit/llvm/llvmjit_types.c
index 47c9daf4023..9fc525cbe45 100644
--- a/src/backend/jit/llvm/llvmjit_types.c
+++ b/src/backend/jit/llvm/llvmjit_types.c
@@ -159,6 +159,7 @@ void *referenced_functions[] =
ExecEvalNextValueExpr,
ExecEvalParamExec,
ExecEvalParamExtern,
+ ExecEvalParamSet,
ExecEvalRow,
ExecEvalRowNotNull,
ExecEvalRowNull,
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index a28ddcdd771..09c48a7ba92 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -160,6 +160,8 @@ typedef enum ExprEvalOp
EEOP_PARAM_EXEC,
EEOP_PARAM_EXTERN,
EEOP_PARAM_CALLBACK,
+ /* set PARAM_EXEC value */
+ EEOP_PARAM_SET,
/* return CaseTestExpr value */
EEOP_CASE_TESTVAL,
@@ -380,7 +382,7 @@ typedef struct ExprEvalStep
ExprEvalRowtypeCache rowcache;
} nulltest_row;
- /* for EEOP_PARAM_EXEC/EXTERN */
+ /* for EEOP_PARAM_EXEC/EXTERN and EEOP_PARAM_SET */
struct
{
int paramid; /* numeric ID for parameter */
@@ -777,6 +779,8 @@ extern void ExecEvalFuncExprStrictFusage(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
+extern void ExecEvalParamSet(ExprState *state, ExprEvalStep *op,
+ ExprContext *econtext);
extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalCoerceViaIOSafe(ExprState *state, ExprEvalStep *op);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 444a5f0fd57..ce93ed451a6 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -959,7 +959,6 @@ typedef struct SubPlanState
struct PlanState *planstate; /* subselect plan's state tree */
struct PlanState *parent; /* parent plan node's state tree */
ExprState *testexpr; /* state of combining expression */
- List *args; /* states of argument expression(s) */
HeapTuple curTuple; /* copy of most recent tuple from subplan */
Datum curArray; /* most recent array from ARRAY() subplan */
/* these are used when hashing the subselect's output: */
--
2.34.1
Alena Rybakina <lena.ribackina@yandex.ru> writes:
I fixed it. The code remains the same.
I see the cfbot is again complaining that this patch doesn't apply.
In hopes of pushing this over the finish line, I fixed up the (minor)
patch conflict and also addressed the cosmetic complaints I had
upthread [1]/messages/by-id/2618533.1677874158@sss.pgh.pa.us. I think the attached v4 is committable. If Andres is
too busy, I can push it, but really it's his patch ...
(BTW, I see no need for additional test cases. Coverage checks show
that all this code is reached during the core regression tests.)
regards, tom lane
Attachments:
v4-0001-Evaluate-arguments-of-correlated-SubPlans-in-the.patchtext/x-diff; charset=us-ascii; name*0=v4-0001-Evaluate-arguments-of-correlated-SubPlans-in-the.pa; name*1=tchDownload
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index ccd4863778..9a13825161 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -69,6 +69,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 ExecInitSubPlanExpr(SubPlan *subplan,
+ ExprState *state,
+ Datum *resv, bool *resnull);
static void ExecCreateExprSetupSteps(ExprState *state, Node *node);
static void ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info);
static bool expr_setup_walker(Node *node, ExprSetupInfo *info);
@@ -1405,7 +1408,6 @@ ExecInitExprRec(Expr *node, ExprState *state,
case T_SubPlan:
{
SubPlan *subplan = (SubPlan *) node;
- SubPlanState *sstate;
/*
* Real execution of a MULTIEXPR SubPlan has already been
@@ -1422,19 +1424,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
break;
}
- 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;
-
- ExprEvalPushStep(state, &scratch);
+ ExecInitSubPlanExpr(subplan, state, resv, resnull);
break;
}
@@ -2714,6 +2704,70 @@ ExecInitFunc(ExprEvalStep *scratch, Expr *node, List *args, Oid funcid,
}
}
+/*
+ * Append the steps necessary for the evaluation of a SubPlan node to
+ * ExprState->steps.
+ *
+ * subplan - SubPlan expression to evaluate
+ * state - ExprState to whose ->steps to append the necessary operations
+ * resv / resnull - where to store the result of the node into
+ */
+static void
+ExecInitSubPlanExpr(SubPlan *subplan,
+ ExprState *state,
+ Datum *resv, bool *resnull)
+{
+ ExprEvalStep scratch = {0};
+ SubPlanState *sstate;
+ ListCell *pvar;
+ ListCell *l;
+
+ if (!state->parent)
+ elog(ERROR, "SubPlan found with no parent plan");
+
+ /*
+ * Generate steps to evaluate input arguments for the subplan.
+ *
+ * We evaluate the argument expressions into ExprState's resvalue/resnull,
+ * and then use PARAM_SET to update the parameter. We do that, instead of
+ * evaluating directly into the param, to avoid depending on the pointer
+ * value remaining stable / being included in the generated expression. No
+ * danger of conflicts with other uses of resvalue/resnull as storing and
+ * using the value always is in subsequent steps.
+ *
+ * Any calculation we have to do can be done in the parent econtext, since
+ * the Param values don't need to have per-query lifetime.
+ */
+ Assert(list_length(subplan->parParam) == list_length(subplan->args));
+ forboth(l, subplan->parParam, pvar, subplan->args)
+ {
+ int paramid = lfirst_int(l);
+ Expr *arg = (Expr *) lfirst(pvar);
+
+ ExecInitExprRec(arg, state,
+ &state->resvalue, &state->resnull);
+
+ scratch.opcode = EEOP_PARAM_SET;
+ scratch.d.param.paramid = paramid;
+ /* paramtype's not actually used, but we might as well fill it */
+ scratch.d.param.paramtype = exprType((Node *) arg);
+ ExprEvalPushStep(state, &scratch);
+ }
+
+ 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.resvalue = resv;
+ scratch.resnull = resnull;
+ scratch.d.subplan.sstate = sstate;
+
+ ExprEvalPushStep(state, &scratch);
+}
+
/*
* Add expression steps performing setup that's needed before any of the
* main execution of the expression.
@@ -2788,29 +2842,12 @@ ExecPushExprSetupSteps(ExprState *state, ExprSetupInfo *info)
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);
+ ExecInitSubPlanExpr(subplan, state,
+ &state->resvalue, &state->resnull);
}
}
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index d8735286c4..acdd918cbb 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -450,6 +450,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
&&CASE_EEOP_PARAM_EXEC,
&&CASE_EEOP_PARAM_EXTERN,
&&CASE_EEOP_PARAM_CALLBACK,
+ &&CASE_EEOP_PARAM_SET,
&&CASE_EEOP_CASE_TESTVAL,
&&CASE_EEOP_MAKE_READONLY,
&&CASE_EEOP_IOCOERCE,
@@ -1093,6 +1094,13 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
EEO_NEXT();
}
+ EEO_CASE(EEOP_PARAM_SET)
+ {
+ /* out of line, unlikely to matter performancewise */
+ ExecEvalParamSet(state, op, econtext);
+ EEO_NEXT();
+ }
+
EEO_CASE(EEOP_CASE_TESTVAL)
{
/*
@@ -2555,6 +2563,24 @@ ExecEvalParamExtern(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
errmsg("no value found for parameter %d", paramId)));
}
+/*
+ * Set value of a param (currently always PARAM_EXEC) from
+ * state->res{value,null}.
+ */
+void
+ExecEvalParamSet(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
+{
+ ParamExecData *prm;
+
+ prm = &(econtext->ecxt_param_exec_vals[op->d.param.paramid]);
+
+ /* Shouldn't have a pending evaluation anymore */
+ Assert(prm->execPlan == NULL);
+
+ prm->value = state->resvalue;
+ prm->isnull = state->resnull;
+}
+
/*
* Evaluate a CoerceViaIO node in soft-error mode.
*
diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c
index 6e48062f56..34f28dfece 100644
--- a/src/backend/executor/execProcnode.c
+++ b/src/backend/executor/execProcnode.c
@@ -393,6 +393,10 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
/*
* Initialize any initPlans present in this node. The planner put them in
* a separate list for us.
+ *
+ * The defining characteristic of initplans is that they don't have
+ * arguments, so we don't need to evaluate them (in contrast to
+ * ExecInitSubPlanExpr()).
*/
subps = NIL;
foreach(l, node->initPlan)
@@ -401,6 +405,7 @@ ExecInitNode(Plan *node, EState *estate, int eflags)
SubPlanState *sstate;
Assert(IsA(subplan, SubPlan));
+ Assert(subplan->args == NIL);
sstate = ExecInitSubPlan(subplan, result);
subps = lappend(subps, sstate);
}
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 9697b1f396..a96cdd01e1 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -107,7 +107,7 @@ ExecHashSubPlan(SubPlanState *node,
TupleTableSlot *slot;
/* Shouldn't have any direct correlation Vars */
- if (subplan->parParam != NIL || node->args != NIL)
+ if (subplan->parParam != NIL || subplan->args != NIL)
elog(ERROR, "hashed subplan with direct correlation not supported");
/*
@@ -231,7 +231,6 @@ ExecScanSubPlan(SubPlanState *node,
TupleTableSlot *slot;
Datum result;
bool found = false; /* true if got at least one subplan tuple */
- ListCell *pvar;
ListCell *l;
ArrayBuildStateAny *astate = NULL;
@@ -248,26 +247,19 @@ ExecScanSubPlan(SubPlanState *node,
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.)
+ * We rely on the caller to evaluate plan correlation values, if
+ * necessary. However we still need to record the fact that the values
+ * (might have) changed, otherwise the ExecReScan() below won't know that
+ * nodes need to be rescanned.
*/
- Assert(list_length(subplan->parParam) == list_length(node->args));
-
- forboth(l, subplan->parParam, pvar, node->args)
+ foreach(l, subplan->parParam)
{
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);
}
- /*
- * Now that we've set up its parameters, we can reset the subplan.
- */
+ /* with that done, we can reset the subplan */
ExecReScan(planstate);
/*
@@ -817,6 +809,10 @@ slotNoNulls(TupleTableSlot *slot)
* as well as regular SubPlans. Note that we don't link the SubPlan into
* the parent's subPlan list, because that shouldn't happen for InitPlans.
* Instead, ExecInitExpr() does that one part.
+ *
+ * We also rely on ExecInitExpr(), more precisely ExecInitSubPlanExpr(), to
+ * evaluate input parameters, as that allows them to be evaluated as part of
+ * the expression referencing the SubPlan.
* ----------------------------------------------------------------
*/
SubPlanState *
@@ -844,7 +840,6 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
/* Initialize subexpressions */
sstate->testexpr = ExecInitExpr((Expr *) subplan->testexpr, parent);
- sstate->args = ExecInitExprList(subplan->args, parent);
/*
* initialize my state
@@ -1107,7 +1102,7 @@ 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)
+ if (subplan->parParam || subplan->args)
elog(ERROR, "correlated subplans should not be executed via ExecSetParamPlan");
/*
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index cbd9ed7cc4..27f94f9007 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1145,6 +1145,12 @@ llvm_compile_expr(ExprState *state)
break;
}
+ case EEOP_PARAM_SET:
+ build_EvalXFunc(b, mod, "ExecEvalParamSet",
+ v_state, op, v_econtext);
+ LLVMBuildBr(b, opblocks[opno + 1]);
+ break;
+
case EEOP_SBSREF_SUBSCRIPTS:
{
int jumpdone = op->d.sbsref_subscript.jumpdone;
diff --git a/src/backend/jit/llvm/llvmjit_types.c b/src/backend/jit/llvm/llvmjit_types.c
index f93c383fd5..4a9e077014 100644
--- a/src/backend/jit/llvm/llvmjit_types.c
+++ b/src/backend/jit/llvm/llvmjit_types.c
@@ -160,6 +160,7 @@ void *referenced_functions[] =
ExecEvalNextValueExpr,
ExecEvalParamExec,
ExecEvalParamExtern,
+ ExecEvalParamSet,
ExecEvalRow,
ExecEvalRowNotNull,
ExecEvalRowNull,
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index 55337d4916..9f520a867a 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -160,6 +160,8 @@ typedef enum ExprEvalOp
EEOP_PARAM_EXEC,
EEOP_PARAM_EXTERN,
EEOP_PARAM_CALLBACK,
+ /* set PARAM_EXEC value */
+ EEOP_PARAM_SET,
/* return CaseTestExpr value */
EEOP_CASE_TESTVAL,
@@ -384,7 +386,7 @@ typedef struct ExprEvalStep
ExprEvalRowtypeCache rowcache;
} nulltest_row;
- /* for EEOP_PARAM_EXEC/EXTERN */
+ /* for EEOP_PARAM_EXEC/EXTERN and EEOP_PARAM_SET */
struct
{
int paramid; /* numeric ID for parameter */
@@ -796,6 +798,8 @@ extern void ExecEvalFuncExprStrictFusage(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
+extern void ExecEvalParamSet(ExprState *state, ExprEvalStep *op,
+ ExprContext *econtext);
extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalCoerceViaIOSafe(ExprState *state, ExprEvalStep *op);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index cac684d9b3..c3670f7158 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -961,7 +961,6 @@ typedef struct SubPlanState
struct PlanState *planstate; /* subselect plan's state tree */
struct PlanState *parent; /* parent plan node's state tree */
ExprState *testexpr; /* state of combining expression */
- List *args; /* states of argument expression(s) */
HeapTuple curTuple; /* copy of most recent tuple from subplan */
Datum curArray; /* most recent array from ARRAY() subplan */
/* these are used when hashing the subselect's output: */
On 18.07.2024 23:01, Tom Lane wrote:
Alena Rybakina <lena.ribackina@yandex.ru> writes:
I fixed it. The code remains the same.
I see the cfbot is again complaining that this patch doesn't apply.
In hopes of pushing this over the finish line, I fixed up the (minor)
patch conflict and also addressed the cosmetic complaints I had
upthread [1]. I think the attached v4 is committable. If Andres is
too busy, I can push it, but really it's his patch ...(BTW, I see no need for additional test cases. Coverage checks show
that all this code is reached during the core regression tests.)regards, tom lane
Thank you for your contribution! I looked at the patch again and I agree
that it is ready to be pushed.
On 2024-07-18 16:01:19 -0400, Tom Lane wrote:
Alena Rybakina <lena.ribackina@yandex.ru> writes:
I fixed it. The code remains the same.
I see the cfbot is again complaining that this patch doesn't apply.
In hopes of pushing this over the finish line, I fixed up the (minor)
patch conflict and also addressed the cosmetic complaints I had
upthread [1]. I think the attached v4 is committable. If Andres is
too busy, I can push it, but really it's his patch ...
Thanks for the rebase - I'll try to get it pushed in the next few days!
Hi,
On 2023-02-25 13:44:01 -0800, Andres Freund wrote:
Ended up simpler than I'd thought. I see small, consistent, speedups and
reductions in memory usage.
For the sake of person following the link from the commit message to this
thread in a few years, I thought it'd be useful to have an example for the
differences due to the patch.
Consider e.g. the query used for psql's \d pg_class, just because that's the
first thing using a subplan that I got my hand on:
Memory usage in ExecutorState changes from
Grand total: 131072 bytes in 12 blocks; 88696 free (2 chunks); 42376 used
to
Grand total: 131072 bytes in 12 blocks; 93656 free (4 chunks); 37416 used
What's more interesting is that if I - just to show the effect - force JITing,
EXPLAIN ANALYZE's jit section changes from:
JIT:
Functions: 31
Options: Inlining true, Optimization true, Expressions true, Deforming true
Timing: Generation 2.656 ms (Deform 1.496 ms), Inlining 25.147 ms, Optimization 112.853 ms, Emission 81.585 ms, Total 222.241 ms
to
JIT:
Functions: 21
Options: Inlining true, Optimization true, Expressions true, Deforming true
Timing: Generation 1.883 ms (Deform 0.990 ms), Inlining 23.821 ms, Optimization 85.150 ms, Emission 64.303 ms, Total 175.157 ms
I.e. noticeably reduced overhead, mostly due to the reduction in emitted
functions.
The difference obviously gets bigger the more parameters the subplan has, in
artificial cases it can be very large.
I also see some small performance gains during execution, but for realistic
queries that's in the ~1-3% range.
Greetings,
Andres Freund
Hi,
On 2024-07-19 21:17:12 -0700, Andres Freund wrote:
On 2024-07-18 16:01:19 -0400, Tom Lane wrote:
Alena Rybakina <lena.ribackina@yandex.ru> writes:
I fixed it. The code remains the same.
I see the cfbot is again complaining that this patch doesn't apply.
In hopes of pushing this over the finish line, I fixed up the (minor)
patch conflict and also addressed the cosmetic complaints I had
upthread [1]. I think the attached v4 is committable. If Andres is
too busy, I can push it, but really it's his patch ...Thanks for the rebase - I'll try to get it pushed in the next few days!
And finally done. No code changes. I did spend some more time evaluating the
resource usage benefits actually do exist (see mail upthread).
Thanks for the reviews, rebasing and the reminders!
Greetings,
Andres