Обсуждение: The const expression evaluation routine should always return a copy

IMO, the routine eval_const_expressions_mutator contains some stale code:

case T_SubPlan:
case T_AlternativeSubPlan:
   /*
    * Return a SubPlan unchanged --- too late to do anything with it.
    *
    * XXX should we ereport() here instead?  Probably this routine
    * should never be invoked after SubPlan creation.
    */
    return node;

At least, this code could be achieved with estimate_expression_value(). 
So, we should fix the comment. But maybe we need to do a bit more. 
According to the mutator idea, only the Query node is returned 
unchanged. If the Subplan node is on top of the expression, the call 
above returns the same node, which may be unconventional.
I'm not totally sure about the impossibility of constantifying SubPlan: 
we already have InitPlans for uncorrelated subplans. Maybe something 
about parameters that can be estimated as constants at this level and, 
as a result, allow to return a Const instead of SubPlan?
But at least we can return a flat copy of the SubPplan node just for the 
convention — the same thing for the AlternativeSubPlan. See the patch in 
the attachment.

-- 
regards,
Andrei Lepikhov
Postgres Professional

Andrei Lepikhov <a.lepikhov@postgrespro.ru> writes:
> IMO, the routine eval_const_expressions_mutator contains some stale code:

I'd like to push back against the idea that eval_const_expressions
is expected to return a freshly-copied tree.  Its API specification
contains no such claim.  It's true that it appears to do that
everywhere but here, but I think that's an implementation detail
that callers had better not depend on.  It's not hard to imagine
someone trying to improve its performance by avoiding unnecessary
copying.

Also, your proposed patch introduces a great deal of schizophrenia,
because SubPlan has substructure.  What's the point of making a copy
of the SubPlan node itself, if the testexpr and args aren't copied?
But we shouldn't modify those, because as the comment states, it's
a bit late to be doing so.

I agree that the comment claiming we can't get here is outdated,
but I'm unexcited about changing the code's behavior.

            regards, tom lane

On 28/2/2024 04:19, Tom Lane wrote:
> Andrei Lepikhov <a.lepikhov@postgrespro.ru> writes:
>> IMO, the routine eval_const_expressions_mutator contains some stale code:
> 
> I'd like to push back against the idea that eval_const_expressions
> is expected to return a freshly-copied tree.  Its API specification
> contains no such claim.  It's true that it appears to do that
> everywhere but here, but I think that's an implementation detail
> that callers had better not depend on.  It's not hard to imagine
> someone trying to improve its performance by avoiding unnecessary
> copying.
Thanks for the explanation. I was just such a developer of extensions 
who had looked into the internals of the eval_const_expressions, found a 
call for a '_mutator' function, and made such a mistake :).
I agree that freeing the return node value doesn't provide a sensible 
benefit because the underlying bushy tree was copied during mutation. 
What's more, it makes even less sense with the selectivity context 
coming shortly (I hope).

-- 
regards,
Andrei Lepikhov
Postgres Professional