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

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.

https://commitfest.postgresql.org/35/3327/

regards,
Ranier Vilela