The bogus calls in remove_self_join_rel()

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

On Thu, Apr 23, 2026 at 12:11 PM David Rowley <dgrowleyml@gmail.com> wrote:

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.

Right. The CTE's reference in the outer query is RTE_CTE, and the
rte->rtekind == RTE_RELATION check ensures SJE never considers it as a
candidate. The CTE itself is planned as a separate Query, where the
varno != root->parse->resultRelation check rules out its target
relation.

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.

Right. And this makes the comment and commit message not accurate, as
inheritance children have not been added yet, as that happens later in
add_other_rels_to_query(). Fixed in v2.

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.

Hmm, I considered that, but I chose the current placement because the
Asserts are documenting a specific non-action: "we don't touch these
two sets, and here is why." That reads more naturally adjacent to the
cleanup of all the other structures, rather than at the top where it
would turn into a precondition claim. The existing Asserts at the top
check input-parameter validity, which is a different kind of check.

On the early-returns argument: remove_self_join_rel() has no early
returns today, and adding one would mean forgetting to clear some
field, so I don't expect that to change.

That said, either location is OK, so happy to move them if you feel
strongly.

It may also be useful to decorate adjust_relid_set() with pg_nodiscard.

Good suggestion. Done in v2.

- Richard

On 23/04/2026 04:45, Richard Guo 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.

There is a clear history of these calls. When designing SJE, we initially
applied it to partitioned tables. Later, we realised complicated issues arise
when SJE meets DML, the RETURNING clause, and partitioned tables. So, we reduced
the feature for some time. I guess the core code's stability has been proven
enough by PG18. We may introduce SJE over partitioned tables in the next release.

You can probably remove these calls for now. Just make sure to add assertions to
help with developing the partitioned case.

--
regards, Andrei Lepikhov,
pgEdge