Hi, On 2023-03-03 15:09:18 -0500, Tom Lane wrote: > Andres Freund <and...@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