Re: Performance issue in foreign-key-aware join estimation

David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> [ eclass_indexes_v4.patch ]

I still don't like this approach too much. I think we can fairly easily
construct the eclass_indexes at a higher level instead of trying to
manage them in add_eq_member, and after some hacking I have the attached.
Some notes:

* To be sure of knowing whether the indexes have been made yet, I added
a new flag variable PlannerInfo.ec_merging_done. This seems like a
cleaner fix for the dependency between canonical-pathkey construction
and EC construction, too. I did need to add code to set the flag in
a couple of short-circuit code paths where we never call
generate_base_implied_equalities(), though.

* I kind of want to rename generate_base_implied_equalities() to
something else, considering that it does a good bit more than just
that now. But I haven't thought of a good name. I considered
finalize_equivalence_classes(), but that seems like an overstatement
when it's still possible for new sort-key eclasses to get made later.

* I did not include your changes to use the indexes in
generate_join_implied_equalities[_for_ecs], because they were wrong.
generate_join_implied_equalities_for_ecs is supposed to consider only
ECs listed in the input list. (It's troublesome that apparently we
don't have any regression tests exposing that need; we should try to
make a test case that does show it.) There's probably some way to
still make use of the indexes there. If nothing else, we could refactor
so that generate_join_implied_equalities isn't just a wrapper around
generate_join_implied_equalities_for_ecs but has its own looping, and
we only use the indexes in generate_join_implied_equalities not the
other entry point. I haven't tried though.

* You mentioned postgres_fdw.c's get_useful_ecs_for_relation as
potentially affected by this change. It looks to me like we could
nuke that function altogether in favor of having its caller scan the
foreign table's eclass_indexes. I haven't tried that either.

I'm unsure how hard we should push to get something like this into v12.
I'm concerned that its dependency on list_nth might result in performance
regressions in some cases; it's a lot easier to believe that this will
be mostly-a-win with the better List infrastructure we're hoping to get
into v13. If you want to keep playing with it, OK, but I'm kind of
tempted to shelve it for now.

regards, tom lane