Robert Haas <[email protected]> writes:
> On Mon, Apr 8, 2024 at 10:42 AM Heikki Linnakangas <[email protected]> wrote:
>> Can you elaborate, which patches you think were not ready? Let's make
>> sure to capture any concrete concerns in the Open Items list.
> Hi,
> I'm moving this topic to a new thread for better visibility and less
> admixture of concerns. I'd like to invite everyone here present to
> opine on which patches we ought to be worried about. Here are a few
> picks from me to start things off. My intention here is to convey "I
> find these scary" rather than "these commits were irresponsible," so I
> respectfully ask that you don't take the choice to list your patch
> here as an attack, or the choice not to list your patch here as an
> endorsement.
I have another one that I'm not terribly happy about:
Author: Alexander Korotkov <[email protected]>
Branch: master [72bd38cc9] 2024-04-08 01:27:52 +0300
Transform OR clauses to ANY expression
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?
* 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.
* 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.
* 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:
* 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.
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.
regards, tom lane
