On Wed, Mar 12, 2025 at 1:32 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On Mon, 10 Mar 2025 at 13:05, Richard Guo <guofenglinux@gmail.com> wrote:
> > Attached are the patches.
> These 2 comment changes from 0002 could be made part of 0001:
>
> 1). In pull_up_simple_subquery(), removing the word "Again" from the
> comment following the deleted block, since this is now the only place
> that sets wrap_non_vars there.
>
> 2). In pull_up_constant_function(), moving "(See comments in
> pull_up_simple_subquery().)" to the next comment.
Nice catch.
> > 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.
> Right. The comment addition in 0002, relating to that, confused me at first:
>
> * This analysis could be tighter: in particular, a non-strict
> * construct hidden within a lower-level PlaceHolderVar is not
> * reason to add another PHV. But for now it doesn't seem
> - * worth the code to be more exact.
> + * worth the code to be more exact. This is also why we need
> + * to handle Vars and PHVs in the above branches, rather than
> + * merging them into this one.
>
> AIUI, it's not really that it *needs* to handle Vars and PHVs
> separately, it's just better if it does, since that avoids
> unnecessarily wrapping a PHV again, if it contains non-strict
> constructs. Also, AFAICS there's no technical reason why simple Vars
> couldn't be handled here (all the tests pass if that branch is
> removed), but as a comment higher up says, that would be more
> expensive. So perhaps this new comment should say "This is why it's
> preferable to handle simple PHVs in the branch above, rather than this
> branch."
Exactly. Technically, we could remove the branch for Vars. However,
I chose to keep it because: 1) it's more efficient this way, and 2)
it's better to keep the handling of Vars and PHVs aligned. I refined
the comment in v2 and ended up with:
* This analysis could be tighter: in particular, a non-strict
* construct hidden within a lower-level PlaceHolderVar is not
* reason to add another PHV. But for now it doesn't seem
- * worth the code to be more exact.
+ * worth the code to be more exact. This is also why it's
+ * preferable to handle bare PHVs in the above branch, rather
+ * than this branch. We also prefer to handle bare Vars in a
+ * separate branch, as it's cheaper this way and parallels the
+ * handling of PHVs.
> Finally, ReplaceWrapOption should be added to pgindent's typedefs.list.
Nice catch.
For backpatching, 0001 seems more like a cosmetic change, and I don't
think it needs to be backpatched. 0002 is a bug fix, but I'm not
confident enough to commit it to the back branches. Given that there
are other wrong result issues with grouping sets fixed in version 18
but not in earlier versions, and that there are no field reports about
this issue, perhaps it's OK to refrain from backpatching this one?
Thanks
Richard
