Обсуждение: 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