Horton

Thread

Commits

  1. Fix comments for ChangeVarNodes() and related functions

  2. Refactor ChangeVarNodesExtended() using the custom callback

  1. Fix comments for ChangeVarNodes() and related functions

    Richard Guo <guofenglinux@gmail.com> — 2025-10-23T08:59:22Z

    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

  2. Re: Fix comments for ChangeVarNodes() and related functions

    Peter Eisentraut <peter@eisentraut.org> — 2025-11-03T12:14:08Z

    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.

  3. Re: Fix comments for ChangeVarNodes() and related functions

    Richard Guo <guofenglinux@gmail.com> — 2025-11-05T03:47:37Z

    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