Re: BUG #19435: Error: "No relation entry for relid 2" Triggered by Complex Join with Self-Referencing Tables

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.)

            regards, tom lane

diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 7249ffbfb36..dc803a17037 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -672,7 +672,7 @@ ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context)
  * value indicating if the given node should be skipped from further processing
  * by ChangeVarNodes_walker.  The callback is called only for expressions and
  * other children nodes of a Query processed by a walker.  Initial processing
- * of the root Query doesn't involve the callback.
+ * of the root Query node doesn't invoke the callback.
  */
 void
 ChangeVarNodesExtended(Node *node, int rt_index, int new_index,
@@ -737,9 +737,16 @@ ChangeVarNodes(Node *node, int rt_index, int new_index, int sublevels_up)
 }

 /*
- * ChangeVarNodesWalkExpression - process expression within the custom
- *                                  callback provided to the
- *                                  ChangeVarNodesExtended.
+ * ChangeVarNodesWalkExpression - process subexpression within a callback
+ *                                  function passed to ChangeVarNodesExtended.
+ *
+ * This is intended to be used by a callback that needs to recursively
+ * process subexpressions of some node being visited by an outer
+ * ChangeVarNodesExtended call (not letting ChangeVarNodes_walker do that).
+ * Hence, we invoke ChangeVarNodes_walker directly.  This means that if
+ * the passed Node is a Query node, it will be treated as a sub-Query,
+ * so sublevels_up will be incremented immediately.  Do not apply this
+ * to a top-level Query node, or you'll likely get wrong results.
  */
 bool
 ChangeVarNodesWalkExpression(Node *node, ChangeVarNodes_context *context)