Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

Andres Freund <andres(at)anarazel(dot)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