Re: Eval expression R/O once time (src/backend/executor/execExpr.c) - Mailing list pgsql-hackers

From Ranier Vilela
Subject Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
Date
Msg-id CAEudQArQSr9=4epP1r2egkrLRi-L2U2Fnr5DMaCs9MNT13XS6w@mail.gmail.com
Whole thread Raw
In response to Re: Eval expression R/O once time (src/backend/executor/execExpr.c)  (Ranier Vilela <ranier.vf@gmail.com>)
List pgsql-hackers
Em ter., 21 de set. de 2021 às 20:12, Ranier Vilela <ranier.vf@gmail.com> escreveu:
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.
Created a new entry at next CF.


regards,
Ranier Vilela

pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: Proposal: Save user's original authenticated identity for logging
Next
From: Juan José Santamaría Flecha
Date:
Subject: Re: Atomic rename feature for Windows.