Re: post-freeze damage control

On 9/4/2024 09:12, Tom Lane wrote:
> I have another one that I'm not terribly happy about:
> 
>      Author: Alexander Korotkov <akorotkov@postgresql.org>
>      Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300
> 
>          Transform OR clauses to ANY expression
Because I'm primary author of the idea, let me answer.
> 
> I don't know that I'd call it scary exactly, but I do think it
> was premature.  A week ago there was no consensus that it was
> ready to commit, but Alexander pushed it (or half of it, anyway)
> despite that.  A few concrete concerns:
> 
> * Yet another planner GUC.  Do we really need or want that?
It is the most interesting question here. Looking around planner 
features designed but not applied for the same reason because they can 
produce suboptimal plans in corner cases, I think about inventing 
flag-type parameters and hiding some features that work better for 
different load types under such flagged parameters.
> 
> * What the medical community would call off-label usage of
> query jumbling.  I'm not sure this is even correct as-used,
> and for sure it's using that code for something never intended.
> Nor is the added code adequately (as in, at all) documented.
I agree with documentation and disagree with critics on the expression 
jumbling. It was introduced in the core. Why don't we allow it to be 
used to speed up machinery with some hashing?
> 
> * Patch refuses to group anything but Consts into the SAOP
> transformation.  I realize that if you want to produce an
> array Const you need Const inputs, but I wonder why it
> wasn't considered to produce an ARRAY[] construct if there
> are available clauses with pseudo-constant (eg Param)
> comparison values.
Good point. I think, we can consider that in the future.
> 
> * I really, really dislike jamming this logic into prepqual.c,
> where it has no business being.  I note that it was shoved
> into process_duplicate_ors without even the courtesy of
> expanding the header comment:
Yeah, I preferred to do it in parse_expr.c with the assumption of some 
'minimal' or 'canonical' tree form. You can see this code in the 
previous version. I think we don't have any bugs here, but we have 
different opinions on how it should work.
> 
>   * process_duplicate_ors
>   *      Given a list of exprs which are ORed together, try to apply
>   *      the inverse OR distributive law.
> 
> Another reason to think this wasn't a very well chosen place is
> that the file's list of #include's went from 4 entries to 11.
> Somebody should have twigged to the idea that this was off-topic
> for prepqual.c.
> 
> * OrClauseGroupKey is not a Node type, so why does it have
> a NodeTag?  I wonder what value will appear in that field,
> and what will happen if the struct is passed to any code
> that expects real Nodes.
It's a hack authored by Alexander. I guess He can provide additional 
reasons in support of that.
> 
> I could probably find some other nits if I spent more time
> on it, but I think these are sufficient to show that this
> was not commit-ready.
It's up to you. On the one hand, I don't see any bugs or strong 
performance issues, and all the issues can be resolved further; on the 
other hand, I've got your good review and some ideas to work out.

-- 
regards,
Andrei Lepikhov
Postgres Professional