public inbox for [email protected]
help / color / mirror / Atom feedFrom: Richard Guo <[email protected]>
To: David Rowley <[email protected]>
Cc: Pg Hackers <[email protected]>
Subject: Re: The bogus calls in remove_self_join_rel()
Date: Thu, 23 Apr 2026 16:58:46 +0900
Message-ID: <CAMbWs48=du2DSGPR_b6AkJ=ecoQ+GcXqzN6XSvn32CHn0x6itA@mail.gmail.com> (raw)
In-Reply-To: <CAApHDvr7c0XP6Hc2P0nqV8iR0mDWUniXXzjiTsgMvpnzePXevg@mail.gmail.com>
References: <CAMbWs49fYQcqJfJ_Gtn8r1GFNoYtb1=2AUab4ieuqY4Zid9ocQ@mail.gmail.com>
<CAApHDvr7c0XP6Hc2P0nqV8iR0mDWUniXXzjiTsgMvpnzePXevg@mail.gmail.com>
On Thu, Apr 23, 2026 at 12:11 PM David Rowley <[email protected]> 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
Attachments:
[application/octet-stream] v2-0001-Fix-bogus-calls-in-remove_self_join_rel.patch (3.6K, 2-v2-0001-Fix-bogus-calls-in-remove_self_join_rel.patch)
download | inline diff:
From 5acb5066d73b738b0861593bbbcf85014f35f8fa Mon Sep 17 00:00:00 2001
From: Richard Guo <[email protected]>
Date: Thu, 23 Apr 2026 11:19:25 +0900
Subject: [PATCH v2] Fix bogus calls in remove_self_join_rel()
remove_self_join_rel() called adjust_relid_set() on all_result_relids
and leaf_result_relids but threw away the return value. Since
adjust_relid_set() returns a freshly-built Relids and does not modify
the input in place, the calls did nothing. This has been the case
since the SJE feature went in (commit fc069a3a6).
There has been no observable misbehavior, because the relid being
passed is guaranteed not to be a member of either set. At the point
remove_self_join_rel() runs, those sets contain only resultRelation;
inheritance children have not been added yet, as that happens later in
query_planner(), in expand_single_inheritance_child() called from
add_other_rels_to_query(). And remove_self_joins_recurse() rejects
parse->resultRelation as an SJE candidate to preserve the EvalPlanQual
mechanism. Even with the result assigned, the calls would be no-ops
in practice.
Rather than make the calls do the cleanup they pretend to do, replace
them with assertions of the invariant. Any future loosening of the
SJE candidate filter -- for instance to allow eliminating a result
relation under provable conditions -- will trip the assertion and
force whoever does it to revisit this code.
Additionally, decorate adjust_relid_set() with pg_nodiscard so that
any future accidental discard of its return value is caught at compile
time.
---
src/backend/optimizer/plan/analyzejoins.c | 12 ++++++++++--
src/include/rewrite/rewriteManip.h | 2 +-
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 03056bdf3e0..0f82ab9facb 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -1994,8 +1994,16 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark,
ChangeVarNodesExtended((Node *) root->processed_tlist, toRemove->relid,
toKeep->relid, 0, replace_relid_callback);
- adjust_relid_set(root->all_result_relids, toRemove->relid, toKeep->relid);
- adjust_relid_set(root->leaf_result_relids, toRemove->relid, toKeep->relid);
+ /*
+ * No need to touch all_result_relids or leaf_result_relids: at this point
+ * those sets contain only parse->resultRelation; inheritance children
+ * have not been added yet; that happens later in add_other_rels_to_query.
+ * And remove_self_joins_recurse rejects parse->resultRelation as an SJE
+ * candidate to preserve the EPQ mechanism. So toRemove->relid cannot be
+ * a member.
+ */
+ Assert(!bms_is_member(toRemove->relid, root->all_result_relids));
+ Assert(!bms_is_member(toRemove->relid, root->leaf_result_relids));
/*
* There may be references to the rel in root->fkey_list, but if so,
diff --git a/src/include/rewrite/rewriteManip.h b/src/include/rewrite/rewriteManip.h
index a6d4e888e06..9d234cb8741 100644
--- a/src/include/rewrite/rewriteManip.h
+++ b/src/include/rewrite/rewriteManip.h
@@ -54,7 +54,7 @@ struct ChangeVarNodes_context
ChangeVarNodes_callback callback;
};
-extern Relids adjust_relid_set(Relids relids, int oldrelid, int newrelid);
+pg_nodiscard extern Relids adjust_relid_set(Relids relids, int oldrelid, int newrelid);
extern void CombineRangeTables(List **dst_rtable, List **dst_perminfos,
List *src_rtable, List *src_perminfos);
extern void OffsetVarNodes(Node *node, int offset, int sublevels_up);
--
2.39.5 (Apple Git-154)
view thread (4+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: The bogus calls in remove_self_join_rel()
In-Reply-To: <CAMbWs48=du2DSGPR_b6AkJ=ecoQ+GcXqzN6XSvn32CHn0x6itA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox