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

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.

> 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.

Hi,

On 2021-09-21 18:21:24 -0400, Tom Lane wrote:
> 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.

Oh, I clearly re-skimmed the code too quickly. Sorry for that!

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.

On Wed, Sep 22, 2021 at 1:12 AM Ranier Vilela <ranier.vf@gmail.com> wrote:
> Anyway, the v1 patch fixes only the expression eval.

The patch looks good to me.

It seems that initially the code looked similar to your patch. See the
commit b8d7f053c5c2bf2a7e8734fe3327f6a8bc711755. Then the variables
were moved to foreach scope by the commit
1ec7679f1b67e84be688a311dce234eeaa1d5de8.

I'll mark the patch as Ready for Commiter.

On 2021-11-02 13:43:46 -0400, Tom Lane wrote:
> Ranier Vilela <ranier.vf@gmail.com> writes:
> > It seems that 1ec7679f1b67e84be688a311dce234eeaa1d5de8 caused the problem.
>
> Indeed.  Fix pushed.

Thanks to both of you!