Re: post-freeze damage control

On Wed, Apr 10, 2024 at 5:27 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Apr 8, 2024 at 10:12 PM Tom Lane <tgl@sss.pgh.pa.us> 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
>
> I realize that this has been reverted now, but what's really
> frustrating about this case is that I reviewed this patch before and
> gave feedback similar to some of the feedback you gave, and it just
> didn't matter, and the patch was committed anyway.
>
> > 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?
>
> IMHO, no, and I said so in
> https://www.postgresql.org/message-id/CA%2BTgmob%3DebuCHFSw327b55DJzE3JtOuZ5owxob%2BMgErb4me_Ag%40mail.gmail.com
>
> > * 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.
>
> And I raised this point here:
> https://www.postgresql.org/message-id/CA%2BTgmoZCgP6FrBQEusn4yaWm02XU8OPeoEMk91q7PRBgwaAkFw%40mail.gmail.com
>
> > * 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.
>
> All of this seems like it might be related to my comments in the above
> email about the transformation being done too early.
>
> > * 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 don't think I raised this issue.
>
> > 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.
>
> Just imagine if someone had taken time to give similar feedback before
> the commit.

FWIW, I made my conclusion that it isn't worth to commit stuff like
this without explicit consent from Tom.  As well as it isn't worth to
commit table AM changes without explicit consent from Andres.  And it
isn't worth it to postpone large features to the last CF (it's better
to postpone to the next release then).

------
Regards,
Alexander Korotkov