Re: [PoC] Reducing planning time when tables have many partitions

On Sat, 1 Mar 2025 at 23:07, Yuya Watari <watari(dot)yuya(at)gmail(dot)com> wrote:
> The previous patches did not apply to the current master, so I have
> rebased them.

Thank you for continuing to work on this. My apologies for having
completely disappeared from this thread for so long.

Looking at v33-0001, there are a few choices you've made that are not
clear to me:

1) Can you describe the difference between
PlannerInfo.top_parent_relid_array and RelOptInfo.top_parent_relids?
If you've added the PlannerInfo field for performance reasons, then
that needs to be documented. I think the bar for adding another field
to do the same thing should be quite high. The
RelOptInfo.top_parent_relids field already is commented with
"redundant, but handy", so having another field in another struct
that's also redundant leads me to think that some design needs more
thought.

If you need a cheap way to take the same shortcut as you're doing in
setup_eclass_child_member_iterator() with "if
(root->top_parent_relid_array == NULL)", then maybe PlannerInfo should
have a boolean field to record if there are any other member rels

2) I think the naming of setup_eclass_child_member_iterator() and
dispose_eclass_child_member_iterator() is confusing. From the names,
I'd expect these to only be returning em_is_child == true members, but
that's not the case.

3) The header comment for setup_eclass_child_member_iterator() does
not seem concise enough. It claims "so that it can iterate over
EquivalenceMembers in 'ec'.", but what does that mean? The definition
of "EquivalenceMembers in 'ec'" isn't clear. Is that just the class's
ec_members, or also the child members that are stored somewhere else.
Users of this function need to know what they'll get so they know
which members they need to ignore or which they can assume won't be
returned. If you don't document that, then it's quite hard to
determine where the faulty code is when we get bugs. The "relids"
parameter needs to be documented too.

4) add_transformed_child_version sounds like it does some
transformation, but all it does is add the EMs for the given
RelOptInfo to the iterator's list. I don't quite follow what's being
"transformed". Maybe there's a better name?

That's all I have for now.

David