On Mon, Apr 8, 2024 at 10:12 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > I have another one that I'm not terribly happy about: > > Author: Alexander Korotkov <akorot...@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
