On 4/4/25 09:37, Richard Guo wrote:
On Wed, Apr 2, 2025 at 10:26 PM Richard Guo <guofengli...@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
