From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
cc: "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: post-freeze damage control
In-reply-to: <CA+TgmoZS9zGgUyFeihqo=piuNhHEiRcYDHXMOkt40dUGx9n8Tg@mail.gmail.com>
References: <CA+TgmoZS9zGgUyFeihqo=piuNhHEiRcYDHXMOkt40dUGx9n8Tg@mail.gmail.com>
Comments: In-reply-to Robert Haas <robertmhaas@gmail.com>
	message dated "Mon, 08 Apr 2024 15:47:34 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <3604468.1712628736.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Mon, 08 Apr 2024 22:12:16 -0400
Message-ID: <3604469.1712628736@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/3604469.1712628736%40sss.pgh.pa.us>
Precedence: bulk

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Apr 8, 2024 at 10:42=E2=80=AFAM 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