Redundant check of em_is_child
In match_eclasses_to_foreign_key_col(), there is this:
if (em->em_is_child)
continue; /* ignore children here */
ISTM, it might as well be:
Assert(!em->em_is_child); /* no children yet */
That's because, I think it's still too early in query_planner() to be
expecting any child EC members.
Thanks,
Amit
Attachments:
fkey-ec-match-no-child-assert-1.patchtext/x-diff; name=fkey-ec-match-no-child-assert-1.patchDownload
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index 67bd760fb4..735bd7fdc6 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -2017,8 +2017,7 @@ match_eclasses_to_foreign_key_col(PlannerInfo *root,
EquivalenceMember *em = (EquivalenceMember *) lfirst(lc2);
Var *var;
- if (em->em_is_child)
- continue; /* ignore children here */
+ Assert(!em->em_is_child); /* no children yet */
/* EM must be a Var, possibly with RelabelType */
var = (Var *) em->em_expr;
On Fri, May 19, 2017 at 3:46 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
In match_eclasses_to_foreign_key_col(), there is this:
if (em->em_is_child)
continue; /* ignore children here */ISTM, it might as well be:
Assert(!em->em_is_child); /* no children yet */
That's because, I think it's still too early in query_planner() to be
expecting any child EC members.
I'm not sure there's really any benefit to this change. In the
future, somebody might want to use the function from someplace later
on in the planner. If the logic as-written would work correctly in
that case now, I can't see why we should turn it into an assertion
failure instead. This isn't really costing us anything, is it?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/06/23 0:00, Robert Haas wrote:
On Fri, May 19, 2017 at 3:46 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:In match_eclasses_to_foreign_key_col(), there is this:
if (em->em_is_child)
continue; /* ignore children here */ISTM, it might as well be:
Assert(!em->em_is_child); /* no children yet */
That's because, I think it's still too early in query_planner() to be
expecting any child EC members.I'm not sure there's really any benefit to this change. In the
future, somebody might want to use the function from someplace later
on in the planner. If the logic as-written would work correctly in
that case now, I can't see why we should turn it into an assertion
failure instead. This isn't really costing us anything, is it?
Not much perhaps. Just thought it might be an oversight (and potentially
a source of confusion today to someone trying to understand this code),
but no harm in leaving it the way it is, as you say, so that someone in
the future doesn't have to worry about checking here.
Sorry for the noise.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers