Re: remove_useless_joins vs. bug #19560

[Richard Guo]

On Mon, Aug 17, 2026 at 8:58 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> One idea for fixing this is to null out the subquery field of a
> subquery RTE as soon as we've pulled it up, but there is probably
> code that will crash on a null subquery pointer.

It seems to me that the subquery in question isn't a pulled-up one.
pull_up_simple_subquery() already sets rte->subquery to NULL at line
prepjointree.c:1725.

What is in question is the subquery of a rel removed by join removal
itself.  Its RTE stays in the rangetable with the query tree intact,
and that tree can contain lateral references to rels that are still
around.  Those references are what prevented the referenced rel from
being removed; once the referencing subquery is gone, the next pass
removes the referenced rel, and remove_rels_from_query_tree() then
walks into the dead subquery and finds the stale Var.  To be concrete,
consider:

  select d.* from d
    left join a on d.a = a.id
    left join lateral (select count(*) from c where c.id = a.b_id) s
      on true;

I think we can just null out the subquery field of a subquery RTE that
gets removed.  We've been doing that for pulled-up subqueries for many
years, so it should be safe.  I mean something like below.

@@ -155,6 +154,10 @@ remove_useless_outer_joins(PlannerInfo *root)
        removed_relids = bms_add_member(removed_relids, innerrelid);
        removed_relids = bms_add_member(removed_relids, sjinfo->ojrelid);

+       /* As in pull_up_simple_subquery, discard a no-longer-needed subquery */
+       if (root->simple_rte_array[innerrelid]->rtekind == RTE_SUBQUERY)
+           root->simple_rte_array[innerrelid]->subquery = NULL;
+

While at it, I still don't think we need ChangeVarNodes() here, since
only PHV phrels can still mention a removed relid.  I think we can
leverage substitute_phv_relids() to do the job, and thus we don't need
duplicate tree-walking code.  The Assert(!IsA(node, AppendRelInfo))
you mentioned is not a problem.  We can handle that by a three-line
loop:

+       foreach_node(AppendRelInfo, appinfo, root->append_rel_list)
+       {
+           Assert(appinfo->parent_relid != relid);
+           Assert(appinfo->child_relid != relid);
+           substitute_phv_relids((Node *) appinfo->translated_vars,
+                                 relid, NULL);
+       }

I agree that there may not be any speed gain, since the walk is the
same.  I think the point is that join removal no longer writes -1 into
varno fields.

Attached is a draft patch showing what I mean.

- Richard

nocfbot.Some-updates-to-v5.patch
Description: Binary data