public inbox for [email protected]
help / color / mirror / Atom feedFrom: Richard Guo <[email protected]>
To: Pg Hackers <[email protected]>
Subject: The bogus calls in remove_self_join_rel()
Date: Thu, 23 Apr 2026 11:45:59 +0900
Message-ID: <CAMbWs49fYQcqJfJ_Gtn8r1GFNoYtb1=2AUab4ieuqY4Zid9ocQ@mail.gmail.com> (raw)
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
Attachments:
[application/octet-stream] v1-0001-Fix-bogus-calls-in-remove_self_join_rel.patch (2.7K, 2-v1-0001-Fix-bogus-calls-in-remove_self_join_rel.patch)
download | inline diff:
From d06fc10fc3a5317a88a0576d9a2cfed1c10a4762 Mon Sep 17 00:00:00 2001
From: Richard Guo <[email protected]>
Date: Thu, 23 Apr 2026 11:19:25 +0900
Subject: [PATCH v1] 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: those sets
contain only the query's top-level target relation, which
remove_self_joins_recurse() rejects as an SJE candidate to preserve
the EvalPlanQual mechanism, plus inheritance children of the target
relation, which are RELOPT_OTHER_MEMBER_RELs and so never appear in
the joinlist that SJE scans for candidates. 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.
---
src/backend/optimizer/plan/analyzejoins.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 03056bdf3e0..494b9ee0716 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: every relid
+ * in those sets is either the query's top-level target relation, which
+ * remove_self_joins_recurse() rejects as an SJE candidate (the EPQ
+ * mechanism requires the target rel to survive), or an inheritance child
+ * of it, which is a RELOPT_OTHER_MEMBER_REL and so never appears in the
+ * joinlist that SJE scans. toRemove->relid therefore 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,
--
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]
Subject: Re: The bogus calls in remove_self_join_rel()
In-Reply-To: <CAMbWs49fYQcqJfJ_Gtn8r1GFNoYtb1=2AUab4ieuqY4Zid9ocQ@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