Hi, Tom!
On Fri, Mar 20, 2026 at 10:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > 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.
>
> After further contemplation I've decided that an Assert would be
> wrong, because it's not impossible that a callback would want
> to invoke this on a sub-Query --- for instance, if it wanted to
> short-circuit ChangeVarNodes's processing of a SubLink node,
> it would need to do that. The key point is that if we do see a
> Query node here, we will treat it as a sub-query not a top-level
> query, which also justifies skipping the work that
> ChangeVarNodesExtended does on a top-level Query. So we just
> need a comment explaining that. I'm thinking about the attached.
>
> (BTW, by this reasoning the previous implementation of
> ChangeVarNodesWalkExpression was doubly wrong, since it would
> have done the wrong thing at a Query node as well as a Var node.)
Thank you so much for caring about this. I agree that this kind of
changes should go with proper comments.
Please, consider my additions to the comment. They explain why we use
ChangeVarNodes_walker() instead of expression_tree_walker(), and gives
a bit more details about difference in processing of top-level Query
and subquery.
------
Regards,
Alexander Korotkov
Supabase
