Alexander Korotkov <aekorotkov@gmail.com> writes:
> OK. I've pushed this.
I don't love this patch. It fixes the functional problem, but it
does nothing to fix the underlying cause of that problem, namely
the abysmal under-documentation of the ChangeVarNodesXXX functions.
There are very specific assumptions about whether the recursion
is starting at a Query or not, and the code will do the wrong
thing if invoked at the wrong node level.
In particular, imagine that ChangeVarNodes_walker is invoked
directly on a Query node, something that ChangeVarNodesExtended
is careful not to do. It will increment sublevels_up immediately
and thus process the contents of the Query with sublevels_up==1,
meaning it will not recognize local Vars as needing adjustment.
At the very least we need to add comments, but I wonder if we
don't actually need an Assert that ChangeVarNodesWalkExpression
is not invoked directly on a Query. It would have done the
right thing before this patch, but now it won't. That's an
okay tradeoff for fixing the bare-Var case, but not documenting
what you did is not okay.
regards, tom lane
