Amit Langote <amitlangot...@gmail.com> writes:
> Attached updated patches.
[ looks at that... ] I seriously, seriously dislike what you did
in build_join_rel, ie adding the new joinrel to the global data
structures before it's fully filled in. That's inevitably going
to bite us on the ass someday, and you couldn't even be bothered
with a comment.
Worse, the reason you did that seems to be so that
generate_join_implied_equalities can find and modify the joinrel,
which is an undocumented and not very well thought out side-effect.
There are other call sites for that where the joinrel may or may not
already exist, meaning that it might or might not add more members into
the joinrel's eclass_indexes. At best that's wasted work, and at
worst it costs efficiency, since we could in principle get different
sets of common relids depending on which join input pair we're
considering. They're equally valid sets, but unioning them is
just going to add more relids than we need.
Also, the existing logic around eclass_indexes is that it's only
set for baserels and we know it is valid after we've finished
EC merging. I don't much like modifying add_child_rel_equivalences
to have some different opinions about that for joinrels.
It'd probably be better to just eat the cost of doing
get_common_eclass_indexes over again when it's time to do
add_child_rel_equivalences for a joinrel, since we aren't (I hope)
going to do that more than once per joinrel anyway. This would
probably require refactoring things so that there are separate
entry points to add child equivalences for base rels and join rels.
But that seems cleaner anyway than what you've got here.
David --- much of the complexity here comes from the addition of
the eclass_indexes infrastructure, so do you have any thoughts?
regards, tom lane
