On Tue, Apr 8, 2025 at 11:12 PM Dean Rasheed <dean.a.rash...@gmail.com> wrote: > On Tue, 8 Apr 2025 at 12:31, Andrei Lepikhov <lepi...@gmail.com> wrote: > > On 4/4/25 09:37, Richard Guo 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. > I recall raising the same concerns when I originally reviewed the > patch. I don't think this code belongs in the rewriter, because it's > very specific to how SJE wants to handle these kinds of nodes. Correct. And I don't think it's this function's responsibility to create new NullTest quals for SJE. > Note also that RestrictInfo is defined in nodes/pathnodes.h, which > describes its contents as internal data structures for the planner, > suggesting that the rewriter has no business processing those kinds of > nodes. > I don't think simply adding a comment addresses those concerns. Agreed. We may need more than just a comment change. > > > 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? > Perhaps the way to do it is to make ChangeVarNodesExtended() take a > callback function to be invoked for each node visited. The callback > (which would then be in the planner, with the other SJE code) would do > any special handling it needed (for RangeTblRef and RestrictInfo > nodes), and call ChangeVarNodes_walker() for any other types of node, > to get the default behaviour. Yeah, this might be a better approach. Perhaps we can borrow some ideas from replace_rte_variables. Thanks Richard
