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
