Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix bogus calls in remove_self_join_rel()

  1. The bogus calls in remove_self_join_rel()

    Richard Guo <guofenglinux@gmail.com> — 2026-04-23T02:45:59Z

    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.

Thoughts?

- Richard

  2. Re: The bogus calls in remove_self_join_rel()

    David Rowley <dgrowleyml@gmail.com> — 2026-04-23T03:11:27Z

    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

  3. Re: The bogus calls in remove_self_join_rel()

    Richard Guo <guofenglinux@gmail.com> — 2026-04-23T07:58:46Z

    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

  4. Re: The bogus calls in remove_self_join_rel()

    Andrei Lepikhov <lepihov@gmail.com> — 2026-04-23T08:11:06Z

    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