Обсуждение: Evaluate arguments of correlated SubPlans in the referencing ExprState
Hi, Around https://www.postgresql.org/message-id/20230224015417.75yimxbksejpffh3%40awork3.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
Вложения
Andres Freund <andres@anarazel.de> writes: > Around > https://www.postgresql.org/message-id/20230224015417.75yimxbksejpffh3%40awork3.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 > > https://www.postgresql.org/message-id/20230224015417.75yimxbksejpffh3%40awork3.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
Вложения
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
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
От
Peter Eisentraut
Дата:
Is this patch still being worked on? 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
Вложения
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]. 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 https://www.postgresql.org/message-id/20240122204117.swton324xcoodnyi%40awork3.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 andattached 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
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 andattached 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
Вложения
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
[1] https://www.postgresql.org/message-id/2618533.1677874158%40sss.pgh.pa.us
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 > > [1] https://www.postgresql.org/message-id/2618533.1677874158%40sss.pgh.pa.us > 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.241ms 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.157ms 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