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

Ranier Vilela <ranier.vf@gmail.com>

From: Ranier Vilela <ranier.vf@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andres Freund <andres@anarazel.de>, Pg Hackers <pgsql-hackers@postgresql.org>
Date: 2021-09-21T23:12:33Z
Lists: pgsql-hackers

Attachments

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

> Andres Freund <andres@anarazel.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

Commits

  1. Fix variable lifespan in ExecInitCoerceToDomain().

  2. expression eval, jit: Minor code cleanups.

  3. Faster expression evaluation and targetlist projection.