From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: pgsql-hackers@postgresql.org
Subject: Re: Evaluate arguments of correlated SubPlans in the referencing
 ExprState
In-reply-to: <20230303001924.tv7m2jlmb3oro4ab@alap3.anarazel.de>
References: <20230225214401.346ancgjqc3zmvek@awork3.anarazel.de>
 <2129737.1677785615@sss.pgh.pa.us>
 <20230302200549.l2ikytmnqzvy5a7a@alap3.anarazel.de>
 <2134061.1677787831@sss.pgh.pa.us>
 <20230302210031.o4vqzrwu3a2kplko@alap3.anarazel.de>
 <20230303001924.tv7m2jlmb3oro4ab@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Thu, 02 Mar 2023 16:19:24 -0800"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2618532.1677874158.1@sss.pgh.pa.us>
Date: Fri, 03 Mar 2023 15:09:18 -0500
Message-ID: <2618533.1677874158@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2618533.1677874158%40sss.pgh.pa.us>
Precedence: bulk

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