Re: v12.0: ERROR: could not find pathkey item to sort - Mailing list pgsql-hackers

On Sun, Nov 3, 2019 at 4:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> 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