On Wed, Mar 5, 2025 at 8:01 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> Yes, that makes sense, and it seems like a fairly straightforward
> simplification, given that perform_pullup_replace_vars() sets it back
> to false shortly afterwards.
>
> The same thing happens in pull_up_constant_function().
Thanks for looking.
Attached are the patches. 0001 removes the code that sets
wrap_non_vars to true for UNION ALL subqueries. And that leaves us
only two cases where we need PHVs for identification purposes:
1. If the query uses grouping sets, we need a PHV for each expression
of the subquery's targetlist items.
2. In the join quals of a full join, we need PHVs for variable-free
expressions.
In 0002, I changed the boolean wrap_non_vars to an enum, which
indicates whether no expressions, all expressions, or only
variable-free expressions need to be wrapped for identification
purposes.
Another thing is that when deciding whether to wrap the newnode in
pullup_replace_vars_callback(), I initially thought that we didn't
need to handle Vars/PHVs specifically, and could instead merge them
into the branch for handling general expressions. However, doing so
causes a plan diff in the regression tests. The reason is that a
non-strict construct hidden within a lower-level PlaceHolderVar can
lead the code for handling general expressions to mistakenly think
that another PHV is needed, even when it isn't. Therefore, the
branches for handling Vars/PHVs are retained in 0002.
Thanks
Richard
