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

Hello David,

Thank you for your prompt response and detailed review. I have
addressed your comments and updated the patches accordingly (attached
as v35).

On Wed, Mar 19, 2025 at 7:48 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> 1. There are various places in equivclass.c that Assert the member
> isn't a child which have a comment "/* no children yet */", e.g.
> generate_base_implied_equalities_const() there's
> "Assert(!cur_em->em_is_child); /* no children yet */". The comment
> implies that ec_members will later contain em_is_child members, but
> that's not true. What you've written in
> generate_implied_equalities_for_column() seems like the correct way to
> handle this, i.e. "Child members should not exist in ec_members"

Thank you for pointing this out. I have updated these assertions and comments.

> 2. There's a comment in setup_eclass_all_member_iterator_for_relids which says:
>
> * whose em_relids is a subset of the given 'child_relids'. The inverted
>
> is 'child_relids' meant to be 'relids'? Otherwise, I don't know what
> 'child_relids' is.

This was my mistake. I have corrected it to 'relids'.

> 3. Can you explain why you've added the following assert to
> reconsider_full_join_clause()?
>
> Assert(!bms_is_empty(coal_em->em_relids));
>
> There are no other changes to that function and I can't quite figure
> out why that Assert is relevant now if it wasn't before. Or is this a
> case of adding additional protection against this? If so, is there
> more risk now than there was before?

I'm sorry, but I cannot recall exactly why I initially added this
assertion. Upon review, I realized it was incorrect and unnecessary,
so I have removed it.

> 4. Is there any point in renaming add_eq_member() to
> add_parent_eq_member()? You don't have a function called
> add_child_eq_member() so is the "parent" word needed here?

I agree with you. I have reverted to the original name, add_eq_member().

> 5. In add_child_join_rel_equivalences() 'lc' can be moved into the
> scope of the while loop.

Fixed.

> 6. The following comment in add_child_join_rel_equivalences() should
> be deleted. That used to be above the "if (cur_em->em_is_child)
> continue;" statement and it does not seem relevant to the code you've
> replaced that with.
>
> /*
> * We consider only original EC members here, not
> * already-transformed child members.
> */

Removed as suggested.

> 7. EquivalenceAllMemberIterator's "modified" field does not seem like
> a great name. Is it better to call this something like
> "list_is_copy"?

Thank you for the suggestion. I have renamed it to "list_is_copy."

> 8. It looks like most of the changes in createplan.c are there because
> you need to get the PlannerInfo down to a few functions where it's
> currently unavailable. I think you should break this out into another
> precursor patch which does this only. It'll be easier to review the
> main patch this way.

I have separated these changes into a distinct precursor patch as suggested.

> 9. The new PlannerInfo parameter in prepare_sort_from_pathkeys() isn't
> documented in the "Input Parameters:" header comment. Likewise for
> make_sort_from_pathkeys() and make_incrementalsort_from_pathkeys()

I have updated the documentation in each of these functions.

> 10. The comment for PlannerInfo.eclass_indexes_array states it's for
> "faster lookups of RestrictInfo". Shouldn't it be "faster
> EquivalenceMember lookups"?
>
> /*
> * eclass_indexes_array is the same length as simple_rel_array and holds
> * the indexes of the corresponding rels for faster lookups of
> * RestrictInfo. See the EquivalenceClass comment for more details.
> */
> struct EquivalenceClassIndexes *eclass_indexes_array
> pg_node_attr(read_write_ignore);

I have revised this comment accordingly.

> I'm also not sure this comment would be accurate enough with only that
> fix as it looks like we don't store any indexes for base rels.

It is true that currently, indexes for EquivalenceMembers do not store
information about base rels. However, the subsequent commit (v35-0004)
introduces indexes for base rels to enable faster RestrictInfo
lookups. Therefore, if we commit the later patch as well, the comment
will remain accurate. What do you think about this?

> 11. In setup_simple_rel_arrays(), is there any point in pallocing
> memory for eclass_indexes_array when root->append_rel_list is empty?

As you mentioned, there is no need to allocate the array in v35-0002
(faster lookups for EquivalenceMembers) when root->append_rel_list is
empty. However, the allocation becomes necessary when introducing
indexes for RestrictInfos in v35-0004. To allow committing only
v35-0002 independently, I now avoid allocating the array when
root->append_rel_list is empty in v35-0002 and instead ensure that it
is always allocated in v35-0004.

> 12. join_rel_list_index isn't being initialized in all the places that
> do makeNode(RelOptInfo); Maybe -1 is a good initial value? (See my
> next point)

Fixed by initializing join_rel_list_index to -1.

> 13. RelOptInfo.join_rel_list_index is an index into
> PlannerInfo.join_rel_list. It shouldn't be of type "Index". "int" is
> the correct type for an index into a List.

Corrected to use "int".

> 14. In add_join_rel(), if you assign the join_rel_list_index before
> the lappend, you don't need to "- 1".

Fixed as suggested.

> I don't currently have the answers you need here. The problem is down
> to how prepunion.c hacks together a targetlist with Vars having
> varno==0. For the union planner work I did last year, to make the
> union planner properly know if a union child had the required
> PathKeys, I had to add EquivalenceMembers for each union child. At
> the moment I don't really know if we could get away with classing
> union children's non-junk target entries as fully-fledged EMs, or if
> these should be child EMs. There's some contradiction as to how the
> RelOptInfo is set up without a parent in recurse_set_operations(), and
> making these child EMs as you're doing in the v34-0002 patch. The
> problem with building the RelOptInfo with a parent is that
> build_simple_rel() would try to apply the same base quals to the child
> as the parent has. That's not the correct thing to do for union
> children as each union child can have different quals. I just don't
> think we have the correct design for the union planner just yet. I
> don't currently have ideas on how to make this better. Maybe
> build_simple_rel() needs some way to distinguish "this rel has a
> parent" and "this rel should copy the parent's quals". However,
> redesigning how this works now likely is a bad idea as it just feels a
> bit late in the release cycle for that. You can see I chickened out
> doing that in 12933dc60, previously 66c0185a3.

I agree it's challenging to redesign the union planner at this stage
of the release cycle. For now, I have proposed a workaround solution
(v35-0003, previously v34-0002). Would this workaround be acceptable
for the current release cycle?

> It's better. These names still feel very long. Also, I don't think the
> iterator struct's name needs to be specific to "AllMembers". Surely
> another setup function could have an iterator loop over any members it
> likes. Likewise, the dispose function does not seem very specific to
> "AllMembers". It's really the setup function that controls which
> members are going to be visited. I suggest the next function, the
> dispose function and the struct are given much more generic names.
> setup_eclass_all_member_iterator_for_relids() feels long. Maybe
> eclass_member_iterator_with_children and forget trying to include
> "relids" in the name?

Following your advice, I have simplified the naming:

* setup_eclass_all_member_iterator_for_relids() -->
setup_eclass_member_iterator_with_children()
* eclass_all_member_iterator_for_relids_next() --> eclass_member_iterator_next()
* dispose_eclass_all_member_iterator() --> dispose_eclass_member_iterator()
* EquivalenceAllMemberIterator --> EquivalenceMemberIterator

===

Thank you again for all your valuable feedback so far.

--
Best regards,
Yuya Watari