Re: post-freeze damage control

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.

--
Robert Haas
EDB: http://www.enterprisedb.com