Thank you for having a look at this.
On Fri, 4 Apr 2025 at 21:47, Amit Langote <amitlangote09@gmail.com> wrote:
> It looks to me like the following hunks in 0002 probably belong in
> 0001, unless you’re planning to commit the patches together anyway:
Ah, yeah. Unsure about that as yet, but I've moved it over.
> The comment on EquivalenceMember might benefit from a mention of how
> ec_childmembers now fits into the picture -- do you think it’s worth
> updating?
>
> /*
> * EquivalenceMember - one member expression of an EquivalenceClass
> *
> * em_is_child signifies that this element was built by transposing a member
> * for an appendrel parent relation to represent the corresponding expression
> * for an appendrel child.
> ...
I've adjusted that a bit in the attached.
> + /* XXX ec_childmembers? */
>
> Maybe we don’t need to print these, since the comment on em_is_child
> suggests they aren’t really full-fledged EC members and are meant to
> be ignored by most operations?
It is marked with pg_node_attr no_read, so I guess that means the
writing is just for debugging since there's nothing else to read it.
In the attached I added a field for the array length and am calling
WRITE_NODE_ARRAY on it.
I spent more time going over all the usages of ec_members. A few
functions do something different to what they did before;
1) print_pathkeys() maybe this should also loop over all child members
too. However, it doesn't seem too important since those are just or
debugging.
2) in convert_subquery_pathkeys() there's some code doing "score =
list_length(outer_ec->ec_members) - 1;", I think this might have
become more correct now that the child members are not contributing to
the score.
I also added a series of Asserts in some places where child members
are not expected yet. analyzejoins.c is doing some fiddling with the
ec_members list, but that's always done before the children are added,
so the Assert is there to make sure that remains true. I didn't see
the sense in writing dead code to remove the child members. I'd feel
more inclined to do that if that code was in equivclass.c
I've attached the updated set of patches. I'm still uncertain what to
do about the EquivalenceMemberIterator returning duplicate members for
child join rels. I'll need to spend more time to see if this is an
actual problem.
David
