Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
Em ter., 2 de nov. de 2021 às 15:33, Andres Freund escreveu: > On 2021-11-02 13:43:46 -0400, Tom Lane wrote: > > Ranier Vilela writes: > > > It seems that 1ec7679f1b67e84be688a311dce234eeaa1d5de8 caused the > problem. > > > > Indeed. Fix pushed. > > Thanks to both of you! > You are welcome, Andres. regards, Ranier Vilela
Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
On 2021-11-02 13:43:46 -0400, Tom Lane wrote: > Ranier Vilela writes: > > It seems that 1ec7679f1b67e84be688a311dce234eeaa1d5de8 caused the problem. > > Indeed. Fix pushed. Thanks to both of you!
Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
Ranier Vilela writes: > It seems that 1ec7679f1b67e84be688a311dce234eeaa1d5de8 caused the problem. Indeed. Fix pushed. regards, tom lane
Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
Em sex., 1 de out. de 2021 às 06:55, Artur Zakirov escreveu: > On Wed, Sep 22, 2021 at 1:12 AM Ranier Vilela 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
Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
On Wed, Sep 22, 2021 at 1:12 AM Ranier Vilela 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
Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
Em ter., 21 de set. de 2021 às 20:12, Ranier Vilela
escreveu:
> Em ter., 21 de set. de 2021 às 19:21, Tom Lane
> escreveu:
>
>> Andres Freund 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
Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
Em ter., 21 de set. de 2021 às 20:00, Andres Freund
escreveu:
> Hi,
>
> On 2021-09-21 18:21:24 -0400, Tom Lane wrote:
> > Andres Freund 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
Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
Em ter., 21 de set. de 2021 às 19:21, Tom Lane escreveu:
> Andres Freund 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
v1_fix_eval_expr_once.patch
Description: Binary data
Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
Hi,
On 2021-09-21 18:21:24 -0400, Tom Lane wrote:
> Andres Freund 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
Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
Andres Freund 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
Re: Eval expression R/O once time (src/backend/executor/execExpr.c)
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
Eval expression R/O once time (src/backend/executor/execExpr.c)
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 fix_eval_expr_once.patch Description: Binary data
