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