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

Hello David,

Thank you for your prompt reply, and apologies for my late response.

On Mon, Mar 24, 2025 at 2:38 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> 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.

Thank you for your advice. I agree that introducing a struct with only
one field is not a good design, so adjusting the patch series to avoid
this issue is necessary.

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

Thank you for pointing this out. This was indeed my mistake, and I
will correct it in the next version of the patch series.

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

I agree. I will fix this comment in the next version.

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

Thank you for clarifying this. It was my oversight. Regarding
speculative execution, I have never measured its impact. I added
"unlikely" based on an assumption that non-partitioned cases would be
common. However, whether this assumption is correct needs to be
discussed.

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

The reason is that some RelOptInfos can be NULL. Further details were
explained in [1]. To be honest, I don't fully understand the
architectural details. Initially, I addressed this by moving the
indexes into RangeTblEntry, but this was not an ideal solution.
Therefore, I moved them into PlannerInfo by introducing a new struct,
"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.

Thank you for your suggestion. Running the benchmarks themselves
should be possible, but given Tom's feedback and the limited time
remaining before feature freeze, it is unlikely that even a partial
integration into v18 is realistic, and a detailed evaluation will
likely need to be deferred until v19. I apologize again for my slow
progress. Given this situation, I plan to carefully reconsider the
overall design and propose a refined patch set for v19. What do you
think about this approach?

Thank you again for your extensive contributions to this patch so far.
I'm sorry that I couldn't get it ready in time for v18.

[1] https://www.postgresql.org/message-id/CAJ2pMkYR_X-%3Dpq%2B39-W5kc0OG7q9u5YUwDBCHnkPur17DXnxuQ%40mail.gmail.com

--
Best regards,
Yuya Watari