public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: Robert Haas <[email protected]>
Cc: [email protected] <[email protected]>
Subject: Re: post-freeze damage control
Date: Mon, 08 Apr 2024 22:12:16 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <CA+TgmoZS9zGgUyFeihqo=piuNhHEiRcYDHXMOkt40dUGx9n8Tg@mail.gmail.com>
References: <CA+TgmoZS9zGgUyFeihqo=piuNhHEiRcYDHXMOkt40dUGx9n8Tg@mail.gmail.com>
Robert Haas <[email protected]> writes:
> On Mon, Apr 8, 2024 at 10:42 AM Heikki Linnakangas <[email protected]> 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 <[email protected]>
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
view thread (11+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: post-freeze damage control
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox