Re: Eval expression R/O once time (src/backend/executor/execExpr.c)

Em ter., 21 de set. de 2021 às 19:21, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> escreveu:

> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2021-09-21 15:09:11 -0300, Ranier Vilela wrote:
> >> Currently when determining where CoerceToDomainValue can be read,
> >> it evaluates every step in a loop.
> >> But, I think that the expression is immutable and should be solved only
> >> once.
>
> > What is immutable here?
>
> I think Ranier has a point here. The clear intent of this bit:
>
> /*
> * If first time through, determine where
> CoerceToDomainValue
> * nodes should read from.
> */
> if (domainval == NULL)
> {
>
> is that we only need to emit the EEOP_MAKE_READONLY once when there are
> multiple CHECK constraints. But because domainval has the wrong lifespan,
> that test is constant-true, and we'll do it over each time to little
> purpose.
>
Exactly, thanks for the clear explanation.

> > And it has to, the allocation intentionally is separate for each
> > constraint. As the comment even explicitly says:
> > /*
> > * Since value might be read multiple times, force
> to R/O
> > * - but only if it could be an expanded datum.
> > */
>
> No, what that's on about is that each constraint might contain multiple
> VALUE symbols. But once we've R/O-ified the datum, we can keep using
> it across VALUE symbols in different CHECK expressions, not just one.
>
> (AFAICS anyway)
>
> I'm unexcited by the proposed move of the save_innermost_domainval/null
> variables, though. It adds no correctness and it forces an additional
> level of indentation of a good deal of code, as the patch fails to show.
>
Ok, but I think that still has a value in reducing the scope.
save_innermost_domainval and save_innermost_domainnull,
only are needed with DOM_CONSTRAINT_CHECK expressions,
and both are declared even when they will not be used.

Anyway, the v1 patch fixes only the expression eval.

regards,
Ranier Vilela