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

Thank you for addressing those comments.

On Mon, 24 Mar 2025 at 12:24, Yuya Watari <watari(dot)yuya(at)gmail(dot)com> wrote:
> 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?

I understand Ashutosh would like to handle the RestrictInfo speedup
another way, so there's additional review work to do there to
determine the merits of each method and figure out the best method.
I'm worried that means we don't get to fix this part for v18 and if
that happens and 0002 goes in alone, then we'd be left with a struct
with a single field. Maybe you should adjust the patch series and
only introduce the new struct in 0004 where it's required.

> 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?

I think something like that is probably ok. You have a problem with
your implementation as you're trying to add the AppendRelInfo once for
each child_tlist element rather than once per union child. Can you fix
this and incorporate into the 0002 patch please?

Here are some more review comments for v35-0002:

1. I don't think the header comment for eclass_member_iterator_next()
needs to mention setup_eclass_member_iterator_with_children(). The
renaming you did in v35 is meant to make it so the
eclass_member_iterator_next and dispose_eclass_member_iterator()
functions don't care about what set up the iterator. We might end up
with new ones in the future and this seems like a comment that might
not get updated when that happens.

2. You should use list_free() in the following:

/*
* XXX Should we use list_free()? I decided to use this style to take
* advantage of speculative execution.
*/
if (unlikely(it->list_is_copy))
pfree(it->ec_members);

The reason is that you're wrongly assuming that calling pfree on the
List pointer is enough to get rid of all memory used by the list. The
List may have a separately allocated elements[] array (this happens
when there's > 5 elements) which you're leaking with the current code.

I assume the speculative execution comment is there because you want
to omit the "list == NULL" check in list_free_private. Is this
measurable, performance-wise?

3. Maybe I'm missing something, but I'm confused about the need for
the eclass_indexes_array field in PlannerInfo. This array is indexed
by the relid, so why can't we get rid of the array and add a field to
RelOptInfo to store the EquivalenceClassIndexes?

4. Could you also please run another set of benchmarks against current
master with the the v36 patches: master, master + v36-0001 + 0002,
master + v36-0001 + 0002 + 0003 (0003 will be the v34-0004 patch), and
then also with v36-0004 (which is the same as v35-0005). The main
thing I'd like to understand here is if there's not enough time to get
the entire patch set committed, is there much benefit to just having
the EquivalenceMember index stuff in by itself without the
RestrictInfo changes.

David