Re: Some problems regarding the self-join elimination code

On Wed, Apr 2, 2025 at 10:26 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> This is not a thorough review of the code; I just randomly looked at
> ChangeVarNodes(). It's possible that there are other issues that
> haven't been discovered, but I think we should at least fix these.
> I'll provide a patch for the fixes later. Apologies for being so
> nitpicky.

With more exposure to the changes in ChangeVarNodes(), I have some
more concerns. As I understand it, ChangeVarNodes() is designed to
update the varno fields of Var nodes in the given tree that belong to
the specific relation; neat and clear. However, now, within this
function, we also create new NullTest quals for SJE, which doesn't
seem like something this function should be handling. It makes this
function look a bit like a kludge.

Actually, while working on the reduce-NullTest-during-constant-folding
patch, I tried to figure out where the new NOT NULL quals from SJE are
added. I was a bit surprised to find that they are added in
ChangeVarNodes. I didn't expect this function to have this extra
responsibility.

Additionally, I have a minor suggestion regarding the new boolean
parameter, "change_RangeTblRef". AFAIU, by convention, we typically
use flags to control the behavior of walker functions, like in
pull_var_clause. While it's fine to use a boolean parameter here
given that we currently only have one flag, for the sake of future
extensibility, I think using flags might be a better choice.

Any ideas on how we could improve this function? Apologies again for
being nitpicky.

Thanks
Richard