Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
On 26.01.2024 05:37, vignesh C wrote:
On Tue, 24 Oct 2023 at 01:47, Alena Rybakina 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
From bf40b14c0cb63f47280299fd3f76a1711db6aada Mon Sep 17 00:00:00 2001
From: Alena Rybakina
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, );
+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 = >resvalue;
- scratch.resnull = >resnull;
-
- ExprEvalPushStep(state, );
+ ExecInitSubPlanExpr(subplan, state,
+ >resvalue, >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 dire
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
On Tue, 24 Oct 2023 at 01:47, Alena Rybakina 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
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
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
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
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.
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
On Tue, Oct 10, 2023 at 10:00 AM Andres Freund wrote: > > Hi, > > On 2023-10-01 14:53:23 -0400, Tom Lane wrote: > > Peter Eisentraut 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? ;-)
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
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
From f7a8ca7f3263fa5f82056f90231cf937133622c9 Mon Sep 17 00:00:00 2001
From: Andres Freund
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, );
+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 = >resvalue;
- scratch.resnull = >resnull;
-
- ExprEvalPushStep(state, );
+ ExecInitSubPlanExpr(subplan, state,
+ >resvalue, >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,
+ >resvalue, >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, );
+ }
+
+ sstate = ExecInitSubPlan(subplan, state->parent);
+
+ /* add SubPlanState nodes to state->parent->sub
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
Hi, On 2023-10-01 14:53:23 -0400, Tom Lane wrote: > Peter Eisentraut 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
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
Peter Eisentraut 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
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
Is this patch still being worked on? On 07.03.23 01:51, Tom Lane wrote: Andres Freund 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
Andres Freund 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
Hi, On 2023-03-03 15:09:18 -0500, Tom Lane wrote: > Andres Freund 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
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
Andres Freund 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
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
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
>From f63915ab00d55b5a67d8b6d05863fe69ea4252b5 Mon Sep 17 00:00:00 2001
From: Andres Freund
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, );
+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 */
- s
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
Hi, On 2023-03-02 15:10:31 -0500, Tom Lane wrote: > Andres Freund 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
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
Andres Freund 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
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
Hi, On 2023-03-02 14:33:35 -0500, Tom Lane wrote: > Andres Freund 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 = >es_param_exec_vals[paramid]; > + > + ExecInitExprRec(lfirst(pvar), state, >value, >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
Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
Andres Freund 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 = >es_param_exec_vals[paramid]; + + ExecInitExprRec(lfirst(pvar), state, >value, >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
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
>From 3b68577bbcd0f78b80abe1ac07eedd6998254951 Mon Sep 17 00:00:00 2001
From: Andres Freund
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, );
+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 = >resvalue;
- scratch.resnull = >resnull;
-
- ExprEvalPushStep(state, );
+ ExecInitSubPlanExpr(subplan, state,
+ >resvalue, >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, s
