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
Date: Fri, 03 Mar 2023 15:09:18 -0500
Message-ID: <2618533.1677874158@sss.pgh.pa.us> (raw)
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de
Subject: Re: Evaluate arguments of correlated SubPlans in the referencing ExprState
In-Reply-To: <2618533.1677874158@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox