Re: [PoC] Reducing planning time when tables have many partitions - Mailing list pgsql-hackers

On Tue, 25 Mar 2025 at 06:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I finally made some time to look at this patchset, and I'm pretty
> disappointed, because after 35 versions I'd expect to see something
> that looks close to committable.  This doesn't really.  I like the
> basic idea of taking child EC members out of ECs' main ec_members
> lists, but there are too many weird details and
> underexplained/overcomplicated/unmaintainable data structures.
>
> One thing I don't love is putting the children into RelOptInfos.
> That seems like an unrelated data structure.  Have you thought
> about instead having, in each EC that needs it, an array indexed
> by RTI of per-relation child-member lists?  I think this might
> net out as less storage because there typically aren't that many
> ECs in a query.  But the main thing is to not have so many
> interconnections between ECs and RelOptInfos.

I think that's quite a good idea. One drawback of that method is that
we'd need to duplicate the EquivalenceMembers into each relid making
up the joinrels in add_child_join_rel_equivalences(). That could mean
finding the same EM multiple times when iterating over the set. I
don't think that causes issues other than wasted effort.

> Another thing I really don't like is the back-link from EMs to ECs:
>
> +       EquivalenceClass *em_ec;        /* EquivalenceClass which has this member */
>
> That makes the data structure circular, which will cause pprint to
> recurse infinitely.  (The fact that you hadn't noticed that makes
> me wonder how you debugged any of these data structure changes.)
> We could prevent the recursion with suitable annotation on this field,
> but I'd really rather not have the field in the first place.  Circular
> pointers are dangerous and best avoided.  Also, it's bloating a node
> type that you are concerned about supporting a lot of.  Another point
> is that I don't see any code to take care of updating these links
> during an EC merge.
>
> Some thoughts about the iterator stuff:
>
> * setup_eclass_member_iterator_with_children is a carpal-tunnel-inducing
> name.  Could we drop the "_with_children" part?  It doesn't seem to
> add much, since there's no variant for "without children".
>
> * The root parameter should be first; IMO there should be no
> exceptions to that within the planner.  Perhaps putting the target
> iterator parameter last would make it read more nicely.  Or you could
> rely on struct assignment:
>
>         it = setup_eclass_member_iterator(root, ec, relids);
>
> * Why did you define the iterator as possibly returning irrelevant
> members?  Doesn't that mean that every caller has to double-check?
> Wouldn't it make for less code and fewer bugs for the iterator to
> have that responsibility?  If there is a good reason to do it like
> that, the comments should explain why.

I've attached 2 patches, which I think addresses most of this, aside
from the last point.

These do need more work. I've just attached what I have so far before
I head off for the day. I am planning on running some performance
tests tomorrow and doing a round on the comments.

> I don't really like the concept of 0004 at all.  Putting *all*
> the EC-related RelOptInfos into a root-stored list seems to be
> doubling down very hard on the assumption that no performance-critical
> operations will ever need to search that whole list.  Is there a good
> reason to do it like that, rather than say using the bitmap-index
> concept separately within each EC?  That might also alleviate the
> problem you're having with the bitmapsets getting too big.

I've dropped this patch out of the set for now. There's other work
going on that might solve the issue that patch was aiming to solve.

> Given that we've only got a week left, I see little hope of getting
> any of this into v18.

I am keen on not giving up quite yet. I'd very much value any further
input you have. It doesn't seem excessively complex to have quite a
large impact on the performance of the planner here.

David