Evaluate arguments of correlated SubPlans in the referencing ExprState

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

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?

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]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

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

[1]: /messages/by-id/2618533.1677874158@sss.pgh.pa.us

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] /messages/by-id/2618533.1677874158@sss.pgh.pa.us

Thank you for your contribution! I looked at the patch again and I agree
that it is ready to be pushed.