On Sun, Nov 3, 2019 at 4:43 AM Tom Lane <[email protected]> wrote: > Amit Langote <[email protected]> writes: > >> 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. > > > Separate entry points sounds better, but only in HEAD? > > I had actually had in mind that we might have two wrappers around a > common search-and-replace routine, but after studying the code I see that > there's just enough differences to make it probably not worth the trouble > to do it like that. I did spend a bit of time removing cosmetic > differences between the two versions, though, mostly by creating > common local variables.
Agree that having two totally separate routines is better.
> I think the way you did the matching_ecs computation is actually wrong:
> we need to find ECs that reference any rel of the join, not only those
> that reference both inputs. If nothing else, the way you have it here
> makes the results dependent on which pair of input rels gets considered
> first, and that's certainly bad for multiway joins.
I'm not sure I fully understand the problems, but maybe you're right.
> Also, I thought we should try to put more conditions on whether we
> invoke add_child_join_rel_equivalences at all. In the attached I did
>
> if ((enable_partitionwise_join || enable_partitionwise_aggregate) &&
> (joinrel->has_eclass_joins ||
> has_useful_pathkeys(root, parent_joinrel)))
>
> but I wonder if we could do more, like checking to see if the parent
> joinrel is partitioned. Alternatively, maybe it's unnecessary because
> we won't try to build child joinrels unless these conditions are true?
Actually, I think we can assert in add_child_rel_equivalences() that
enable_partitionwise_join is true. Then checking
enable_partitionwise_aggregate is unnecessary.
> I did not like the test case either. Creating a whole new (and rather
> large) test table just for this one case is unreasonably expensive ---
> it about doubles the runtime of the equivclass test for me. There's
> already a perfectly good test table in partition_join.sql, which seems
> like a more natural home for this anyhow. After a bit of finagling
> I was able to adapt the test query to fail on that table.
That's great. I tried but I can only finagle so much when it comes to
twisting around plan shapes to what I need. :)
> Patch v4 attached. I've not looked at what we need to do to make this
> work in v12.
Thanks a lot for the revised patch.
Maybe the only difference between HEAD and v12 is that the former has
eclass_indexes infrastructure, whereas the latter doesn't? I have
attached a version of your patch adapted for v12.
Also, looking at this in the patched code:
+ /*
+ * We may ignore expressions that reference a single baserel,
+ * because add_child_rel_equivalences should have handled them.
+ */
+ if (bms_membership(cur_em->em_relids) != BMS_MULTIPLE)
+ continue;
I have been thinking maybe add_child_rel_equivalences() doesn't need
to translate EC members that reference multiple appendrels, because
there top_parent_relids is always a singleton set, whereas em_relids
of such expressions is not? Those half-translated expressions are
useless, only adding to the overhead of scanning ec_members. I'm
thinking that we should apply this diff:
diff --git a/src/backend/optimizer/path/equivclass.c
b/src/backend/optimizer/path/equivclass.c
index e8e9e9a314..d4d80c8101 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -2169,7 +2169,7 @@ add_child_rel_equivalences(PlannerInfo *root,
continue; /* ignore children here */
/* Does this member reference child's topmost parent rel? */
- if (bms_overlap(cur_em->em_relids, top_parent_relids))
+ if (bms_is_subset(cur_em->em_relids, top_parent_relids))
{
/* Yes, generate transformed child version */
Expr *child_expr;
Thoughts?
Thanks,
Amit
d25ea01275-fixup-PG12-v4.patch
Description: Binary data
