Fix comments for ChangeVarNodes() and related functions
I happend to notice this comment for ChangeVarNodes():
* Specifying 'change_RangeTblRef' to false allows skipping RangeTblRef.
* See ChangeVarNodesExtended for details.
However, I couldn't find "change_RangeTblRef" anywhere in the code
base. I suspect this is a leftover from development. In any case, I
think we should fix it.
Also, the comment for ChangeVarNodesExtended() contains an extra
space.
* ChangeVarNodesExtended - similar to ChangeVarNodes, but with an additional
In addition, the comment for replace_relid_callback() has an odd line
wrap.
* SJE needs to skip the RangeTblRef node
* type. During SJE's last step, remove_rel_from_joinlist() removes
And one comment within replace_relid_callback() says:
* as "t1.a" is not null. We use qual() to check for such a case,
I believe it should be "equal" rather than "qual".
Attached is a proposed patch with the fixes. It also includes some
sentence revisions for smoother wording.
FWIW, I think the comment inside remove_rel_from_query() is also a
mess. For example, one comment in this function states:
* Finally, we must recompute per-Var attr_needed and per-PlaceHolderVar
* ph_needed relid sets.
However, there is no corresponding code in this function that
recomputes the attr_needed or ph_needed bits; that recomputation
happens elsewhere.
Another comment in this function states:
* fail to remove other now-removable outer joins. And our removal of the
* join clause(s) for this outer join may mean that Vars that were
* formerly needed no longer are.
However, this function is now used not only to remove outer joins but
also inner (self) joins. So the phrase "for this outer join" in the
comment is misleading.
However, it seems to me that it would be better to address the
comments for remove_rel_from_query() in a separate patch.
- Richard
Attachments:
v1-0001-Fix-comments-for-ChangeVarNodes-and-related-funct.patchapplication/octet-stream; name=v1-0001-Fix-comments-for-ChangeVarNodes-and-related-funct.patchDownload
From e98caffe36a6f307f5163d0035fe6b8255bcb511 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Thu, 23 Oct 2025 17:23:07 +0900
Subject: [PATCH v1] Fix comments for ChangeVarNodes() and related functions
---
src/backend/optimizer/plan/analyzejoins.c | 18 +++++++++---------
src/backend/rewrite/rewriteManip.c | 19 ++++++++-----------
2 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 6a3c030e8ef..e592e1ac3d1 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -1683,14 +1683,14 @@ add_non_redundant_clauses(PlannerInfo *root,
}
/*
- * A custom callback for ChangeVarNodesExtended() providing
- * Self-join elimination (SJE) related functionality
+ * A custom callback for ChangeVarNodesExtended() providing Self-join
+ * elimination (SJE) related functionality
*
- * SJE needs to skip the RangeTblRef node
- * type. During SJE's last step, remove_rel_from_joinlist() removes
- * remaining RangeTblRefs with target relid. If ChangeVarNodes() replaces
- * the target relid before, remove_rel_from_joinlist() fails to identify
- * the nodes to delete.
+ * SJE needs to skip the RangeTblRef node type. During SJE's last
+ * step, remove_rel_from_joinlist() removes remaining RangeTblRefs
+ * with target relid. If ChangeVarNodes() replaces the target relid
+ * before, remove_rel_from_joinlist() would fail to identify the nodes
+ * to delete.
*
* SJE also needs to change the relids within RestrictInfo's.
*/
@@ -1769,8 +1769,8 @@ replace_relid_callback(Node *node, ChangeVarNodes_context *context)
/*
* For self-join elimination, changing varnos could transform
* "t1.a = t2.a" into "t1.a = t1.a". That is always true as long
- * as "t1.a" is not null. We use qual() to check for such a case,
- * and then we replace the qual for a check for not null
+ * as "t1.a" is not null. We use equal() to check for such a
+ * case, and then we replace the qual with a check for not null
* (NullTest).
*/
if (leftOp != NULL && equal(leftOp, rightOp))
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index cd786aa4112..0fcd1fbd14e 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -542,8 +542,6 @@ offset_relid_set(Relids relids, int offset)
* (identified by sublevels_up and rt_index), and change their varno fields
* to 'new_index'. The varnosyn fields are changed too. Also, adjust other
* nodes that contain rangetable indexes, such as RangeTblRef and JoinExpr.
- * Specifying 'change_RangeTblRef' to false allows skipping RangeTblRef.
- * See ChangeVarNodesExtended for details.
*
* NOTE: although this has the form of a walker, we cheat and modify the
* nodes in-place. The given expression tree should have been copied
@@ -664,17 +662,16 @@ ChangeVarNodes_walker(Node *node, ChangeVarNodes_context *context)
}
/*
- * ChangeVarNodesExtended - similar to ChangeVarNodes, but with an additional
+ * ChangeVarNodesExtended - similar to ChangeVarNodes, but with an additional
* 'callback' param
*
- * ChangeVarNodes changes a given node and all of its underlying nodes.
- * This version of function additionally takes a callback, which has a
- * chance to process a node before ChangeVarNodes_walker. A callback
- * returns a boolean value indicating if 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.
+ * ChangeVarNodes changes a given node and all of its underlying nodes. This
+ * version of function additionally takes a callback, which has a chance to
+ * process a node before ChangeVarNodes_walker. A callback returns a boolean
+ * 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.
*/
void
ChangeVarNodesExtended(Node *node, int rt_index, int new_index,
--
2.39.5 (Apple Git-154)
On 23.10.25 10:59, Richard Guo wrote:
I happend to notice this comment for ChangeVarNodes():
* Specifying 'change_RangeTblRef' to false allows skipping RangeTblRef.
* See ChangeVarNodesExtended for details.However, I couldn't find "change_RangeTblRef" anywhere in the code
base. I suspect this is a leftover from development. In any case, I
think we should fix it.
It appears that change_RangeTblRef was removed in commit ab42d643c14 but
the comments were not fully adjusted.
On Mon, Nov 3, 2025 at 9:14 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 23.10.25 10:59, Richard Guo wrote:
I happend to notice this comment for ChangeVarNodes():
* Specifying 'change_RangeTblRef' to false allows skipping RangeTblRef.
* See ChangeVarNodesExtended for details.However, I couldn't find "change_RangeTblRef" anywhere in the code
base. I suspect this is a leftover from development. In any case, I
think we should fix it.
It appears that change_RangeTblRef was removed in commit ab42d643c14 but
the comments were not fully adjusted.
Right. I've pushed this patch and backpatched it to v18. However,
this only covers ChangeVarNodes(). We still need a separate patch to
fix remove_rel_from_query().
- Richard