On Sat, 5 Apr 2025 at 04:05, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This patchset has a distinct whiff of unseemly haste.
hmm, yes. I would like to give this patch as good a chance at making
v18 as I can, and I admit to having optimised for that. Seemingly,
we've got a few other good partitioning performance patches in v18,
and more workloads are now bottlenecked on what this patch aims to fix
than ever before. What I'm aiming to avoid here is tuning those
optimisations to cloud my judgment on the quality of the patch. So,
I'm happy to have your 2nd opinion here.
> 1. The commit message for 0002 still claims that child EC members
> are kept in RelOptInfos, precisely the point I objected to upthread.
> I see that in fact that's untrue, but it'd be nice if the commit log
> had some connection to what's being committed.
Now adjusted.
> 2. Because there is no longer any need to find RelOptInfos, the
> EquivalenceMemberIterator stuff doesn't need a "root" pointer,
> either in the struct or as an setup_eclass_member_iterator argument.
>
> 3. Because of #2, the 0001 patch is useless code churn and should
> be dropped.
I'm glad that's not needed now. Thanks for noticing. Fixed.
> I do note that add_child_eq_member seems to have a considerable
> amount of faith that root->simple_rel_array_size can't increase
> after we start adding child members. That seems rather unsafe,
> though the fact that it hasn't crashed in light testing suggests
> that maybe there's something I'm missing. I would be much
> happier if there were provision to expand the array at need.
I think it's probably worth making that safer.
add_child_rel_equivalences() is currently called after
add_other_rels_to_query(). It is a similar story in the union planner
for add_setop_child_rel_equivalences(), but that's likely no reason to
not be a bit more cautious.
I am still thinking about the duplicate members being returned from
the iterator for child join rels due to them being duplicated into
each component relid element in ec_childmembers. I did consider if
these could just not be duplicated and instead just put into the
ec_childmember element according to their lowest component relid. For
that to work, all callers that need these would need to ensure they
never pass some subset of child_relids when setting up the
EquivalenceMemberIterator. I need to study a bit more to understand if
that's doable.
In the meantime, I've attached v40 with a rewritten commit message, a
bit more adjustment to comments and a slightly revised version of
eclass_member_iterator_next() to get rid of some gotos and hopefully
make it easier to follow the logic.
Thank you for looking.
David
