On Thu, Aug 27, 2026 at 2:25 AM Tom Lane <[email protected]> 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-4jn51wovh86hmsmmdlfnvxzrav8khshhp6qgmbtl...@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
nocfbot.delta-v6.patch
Description: Binary data
