On Thu, 23 Apr 2026 at 14:46, Richard Guo <[email protected]> wrote: > > I noticed these two calls in remove_self_join_rel(): > > adjust_relid_set(root->all_result_relids, toRemove->relid, toKeep->relid); > adjust_relid_set(root->leaf_result_relids, toRemove->relid, > toKeep->relid); > > There's no comment explaining them, and as far as I can tell they do > nothing: adjust_relid_set returns a Relids and does not modify the > input in place. > > Rather than make the calls do the cleanup they pretend to do, I think > a better way is to replace them with assertions: toRemove->relid is > not a member of either set. This is true as these two sets contain > only parse->resultRelation (rejected as an SJE candidate to preserve > EvalPlanQual) and inheritance children of the target, which never > appear in the joinlist that SJE scans for candidates.
Yeah, it certainly shouldn't be removing any result relations. I see there's a check in remove_self_joins_recurse() for varno != root->parse->resultRelation. Have you followed through on what happens for CTEs that do DML and RETURNING? I assume that fails on the nearby rte->relkind == RELKIND_RELATION, but I didn't debug to check. The only place I see all_result_relids being added to, aside from the initial setting with bms_make_singleton() is for the inheritance expansion in expand_single_inheritance_child(), which happens after join removals. For leaf_result_relids, it's similar. I think the Asserts should go at the top of the function next to the other Asserts. Putting them near the top makes it clearer when reading code. The Asserts will be very close to the function's header comment, so it's easier to get a picture about what the function supports and does, plus, it helps ensure we still get the Asserts before any early returns are taken. It may also be useful to decorate adjust_relid_set() with pg_nodiscard. David
