On 9/4/2024 09:12, Tom Lane wrote:
I have another one that I'm not terribly happy about:
Author: Alexander Korotkov <akorot...@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
