Re: post-freeze damage control

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Apr 8, 2024 at 10:42 AM Heikki Linnakangas <hlinnaka@iki.fi> 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 <akorotkov@postgresql.org>
    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