Re: Wrong results with subquery pullup and grouping sets

On Mon, 10 Mar 2025 at 13:05, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
> Attached are the patches.
>

This looks good to me. I did some more testing, and I wasn't able to break it.

Some minor nitpicks:

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.

> 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."

Finally, ReplaceWrapOption should be added to pgindent's typedefs.list.

Regards,
Dean