On Thu, 23 Apr 2026 at 14:46, Richard Guo <guofenglinux@gmail.com> 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
