Horton

Thread

Commits

  1. Fix variable lifespan in ExecInitCoerceToDomain().

  2. expression eval, jit: Minor code cleanups.

  3. Faster expression evaluation and targetlist projection.

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

    Ranier Vilela <ranier.vf@gmail.com> — 2021-09-21T18:09:11Z

    Hi,

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.

Otherwise the logic is wrong since by the rules of C, even though the
variable is
being initialized in the declaration, it still receives initialization at
each repetition.
What causes palloc running multiple times.

In other words:
Datum   *domainval = NULL;

is the same:
Datum   *domainval;
domainval = NULL;

Once there, reduce the scope for save_innermost_domainval and
save_innermost_domainnull.

Thoughts?

regards,
Ranier Vilela

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

    Andres Freund <andres@anarazel.de> — 2021-09-21T20:19:32Z

    Hi,

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?


> Otherwise the logic is wrong since by the rules of C, even though the
> variable is
> being initialized in the declaration, it still receives initialization at
> each repetition.
> What causes palloc running multiple times.
> 
> In other words:
> Datum   *domainval = NULL;
> 
> is the same:
> Datum   *domainval;
> domainval = NULL;

Obviously?


> Thoughts?

I don't see what this is supposed to achieve. The allocation of
domainval/domainnull happens on every loop iteration with/without your patch.

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.
					 */
Greetings,

Andres Freund

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-09-21T22:21:24Z

    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.

			regards, tom lane

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

    Andres Freund <andres@anarazel.de> — 2021-09-21T23:00:55Z

    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!


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

Yea.

Greetings,

Andres Freund

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

    Ranier Vilela <ranier.vf@gmail.com> — 2021-09-21T23:12:33Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2021-09-21T23:13:13Z

    Em ter., 21 de set. de 2021 às 20:00, Andres Freund <andres@anarazel.de>
escreveu:

> 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!
>
No problem, thanks for taking a look.

regards,
Ranier Vilela

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

    Ranier Vilela <ranier.vf@gmail.com> — 2021-09-23T11:17:31Z

    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

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

    Artur Zakirov <zaartur@gmail.com> — 2021-10-01T09:55:37Z

    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.

-- 
Artur

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

    Ranier Vilela <ranier.vf@gmail.com> — 2021-10-01T10:04:03Z

    Em sex., 1 de out. de 2021 às 06:55, Artur Zakirov <zaartur@gmail.com>
escreveu:

> 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.
>
Thanks for the search.
It seems that 1ec7679f1b67e84be688a311dce234eeaa1d5de8 caused the problem.


> I'll mark the patch as Ready for Commiter.
>
Thank you.

regards,
Ranier Vilela

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-02T17:43:46Z

    Ranier Vilela <ranier.vf@gmail.com> writes:
> It seems that 1ec7679f1b67e84be688a311dce234eeaa1d5de8 caused the problem.

Indeed.  Fix pushed.

			regards, tom lane

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

    Andres Freund <andres@anarazel.de> — 2021-11-02T18:33:31Z

    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!

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

    Ranier Vilela <ranier.vf@gmail.com> — 2021-11-03T11:44:50Z

    Em ter., 2 de nov. de 2021 às 15:33, Andres Freund <andres@anarazel.de>
escreveu:

> 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!
>
You are welcome, Andres.

regards,
Ranier Vilela