Thread: 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
Attachment
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
Attachment
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