On 4/4/25 09:37, Richard Guo wrote:
> On Wed, Apr 2, 2025 at 10:26 PM Richard Guo <guofenglinux@gmail.com> wrote:
> 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.
To be precise, inside the function we replace old clause with the
NullTest, because relid replacement action made it degenerate. It seems
essential to me and I think it may be ok to add a comment at the top of
ChangeVarNodes, describing this minor optimisation.
> 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.
That was exactly why I wasn't happy with replacing the change_relid
routine with ChangeVarNodes.
But variant with a flag seems non-trivial to implement. Do you have any
proposal about the code?
--
regards, Andrei Lepikhov
