Обсуждение: Optimize constant MinMax expressions

I was working on a little thing where I needed to simulate BETWEEN
SYMMETRIC so naturally I used least() and greatest().  I was a little
surprised to see that my expressions were not folded into straight
constants and the estimates were way off as a consequence.

I came up with the attached patch to fix it, but it's so ridiculously
small that I fear I'm missing something.

I don't think this needs any documentation and I didn't see where we
have any existing tests for eval_const_expressions so I didn't create
any either.

Thoughts?
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Vik Fearing <vik.fearing@2ndquadrant.com> writes:
> I was working on a little thing where I needed to simulate BETWEEN
> SYMMETRIC so naturally I used least() and greatest().  I was a little
> surprised to see that my expressions were not folded into straight
> constants and the estimates were way off as a consequence.

> I came up with the attached patch to fix it, but it's so ridiculously
> small that I fear I'm missing something.

Well, the question this is begging is in the adjacent comment:

                 * Generic handling for node types whose own processing is
                 * known to be immutable, and for which we need no smarts

Can we assume that the underlying datatype comparison function is
immutable?  I guess so, since we assume that in nearby code such as
contain_mutable_functions_walker, but I don't think it should be done
without at least a comment.

BTW, poking around for other code involving MinMaxExpr, I notice that
contain_leaked_vars_walker is effectively assuming that all datatype
comparison functions are leakproof, an assumption I find a bit debatable.
Maybe it's all right, but again, it should certainly not have gone without
a comment.

            regards, tom lane

On 30/12/2018 00:36, Tom Lane wrote:
> Vik Fearing <vik.fearing@2ndquadrant.com> writes:
>> I was working on a little thing where I needed to simulate BETWEEN
>> SYMMETRIC so naturally I used least() and greatest().  I was a little
>> surprised to see that my expressions were not folded into straight
>> constants and the estimates were way off as a consequence.
> 
>> I came up with the attached patch to fix it, but it's so ridiculously
>> small that I fear I'm missing something.
> 
> Well, the question this is begging is in the adjacent comment:
> 
>                  * Generic handling for node types whose own processing is
>                  * known to be immutable, and for which we need no smarts
> 
> Can we assume that the underlying datatype comparison function is
> immutable?  I guess so, since we assume that in nearby code such as
> contain_mutable_functions_walker, but I don't think it should be done
> without at least a comment.

Adding a comment is easy enough.  How is the attached?

> BTW, poking around for other code involving MinMaxExpr, I notice that
> contain_leaked_vars_walker is effectively assuming that all datatype
> comparison functions are leakproof, an assumption I find a bit debatable.
> Maybe it's all right, but again, it should certainly not have gone without
> a comment.

Surely this is out of scope for my patch?
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Vik Fearing <vik.fearing@2ndquadrant.com> writes:
> On 30/12/2018 00:36, Tom Lane wrote:
>> Can we assume that the underlying datatype comparison function is
>> immutable?  I guess so, since we assume that in nearby code such as
>> contain_mutable_functions_walker, but I don't think it should be done
>> without at least a comment.

> Adding a comment is easy enough.  How is the attached?

Pushed with a bit of wordsmithing on the comment.

>> BTW, poking around for other code involving MinMaxExpr, I notice that
>> contain_leaked_vars_walker is effectively assuming that all datatype
>> comparison functions are leakproof, an assumption I find a bit debatable.
>> Maybe it's all right, but again, it should certainly not have gone without
>> a comment.

> Surely this is out of scope for my patch?

I'd been thinking that we might just add a similar comment there, but
on reflection that doesn't seem like the right thing, so I started a
separate thread about it.

            regards, tom lane

On 30/12/2018 19:44, Tom Lane wrote:
> Vik Fearing <vik.fearing@2ndquadrant.com> writes:
>> On 30/12/2018 00:36, Tom Lane wrote:
>>> Can we assume that the underlying datatype comparison function is
>>> immutable?  I guess so, since we assume that in nearby code such as
>>> contain_mutable_functions_walker, but I don't think it should be done
>>> without at least a comment.
> 
>> Adding a comment is easy enough.  How is the attached?
> 
> Pushed with a bit of wordsmithing on the comment.

Thanks!  I've updated the commitfest entry to reflect that.
-- 
Vik Fearing                                          +33 6 46 75 15 36
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support