Re: remove_useless_joins vs. bug #19560

On Thu, Aug 27, 2026 at 2:25 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I still don't like replacing this usage of ChangeVarNodes with
> substitute_phv_relids though. While it's a kluge as-presented,
> I'm not sure that using substitute_phv_relids that way is much less
> of a kluge. Moreover, if there were any leftover Vars in the tree,
> substitute_phv_relids wouldn't notice. I think noticing that is a
> good thing, and the fact that it identified this oversight in
> remove_useless_outer_joins seems like evidence in favor of that.

That is a fair point. The only rels we can remove are plain relations
and subqueries, and the subqueries are now set to NULL, so a Var of a
removed rel should never be left in the tree. It is good to have the
Asserts to check that.

> So what I'd prefer to do is as in 0002 attached. For simplicity of
> review, 0001 is the same as the v5 combined patch I presented before.
> (But it's rebased onto today's HEAD, which changes only line numbers,
> and I tweaked the commit message's claim about planning time per your
> other comments.) 0002 changes ChangeVarNodes' API to have a clean
> way to represent "we're removing this relid", and adds the fix you
> identified with a more extensive comment.

I reviewed v6 as a whole and it LGTM. I have a few small changes to
suggest, see the attached delta on top of v6.

* The first one is to initialize all_result_relids/leaf_result_relids
in subquery_planner like we did before. v5 moved that into
query_planner, but as far as I can tell the only reason was the Assert
in the old self-join code, which is gone now. And I think the current
initialization can contradict the comment in pathnodes.h, as I
mentioned in [1]:point 1.

* In reduce_unique_semijoins, I think we should change the singleton
test to use syn_righthand, as I mentioned in [1]:point 3.

* A nit in the ChangeVarNodes comment about INVALID_VAR. It says
rt_index should only be found in nullingrels sets, but a removed base
relid can also be in a PlaceHolderVar's phrels, which the comment in
remove_rels_from_query_tree already says.

* The Assert on resultRelation in remove_useless_outer_joins can be
dropped, because join_is_removable already rejects that case.

[1] https://postgr.es/m/CAMbWs4-4JN51woVH86hMSMmdLfNVXzRav8KHShhP6QGmbtLNDg@mail.gmail.com

> (Maybe it'd make sense to use this version of ChangeVarNodes in
> prepjointree too? I didn't look into that.)

I don't think so. The two callers of substitute_phv_relids in
prepjointree.c replace a relid with a set of relids, and
ChangeVarNodes can only take one new relid. There is no place in
prepjointree.c that just deletes a relid.

- Richard