On Sat, 5 Apr 2025 at 16:55, David Rowley <dgrowle...@gmail.com> wrote: > 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.
It looks like the child members added by add_child_join_rel_equivalences() are only required for pathkey requirements. The EquivalenceMember mentions: * for an appendrel child. These members are used for determining the * pathkeys of scans on the child relation and for explicitly sorting the * child when necessary to build a MergeAppend path for the whole appendrel * tree. An em_is_child member has no impact on the properties of the EC as a I used the attached .txt file to highlight the places where the iterator returned the same member twice and saw only that find_ec_member_matching_expr() does this. Because during createplan, we'll have the specific RelOptInfo that we need the EquivalenceMember for, we'll be passing the "relids" of that RelOptInfo to setup_eclass_member_iterator(), in which case, I think it's fine to store the member in just one of the ec_childmembers[] array slots for just one of the relids making up the RELOPT_OTHER_JOINREL's component relids as that means it'll be found once only due to how eclass_member_iterator_next() looks at all of the ec_childmembers[] elements for the given relids. Doing this also allows the code in add_child_eq_member() to be simplified. I made this happen in the attached v41 patch, and that's the last outstanding issue that I had for this. I think this is worthy of getting into v18. Does anyone else think differently? It'd be good to know that soon. David
diff --git b/src/backend/optimizer/path/equivclass.c
a/src/backend/optimizer/path/equivclass.c
index b8c64d80987..30f1ca37e2e 100644
--- b/src/backend/optimizer/path/equivclass.c
+++ a/src/backend/optimizer/path/equivclass.c
@@ -617,6 +617,7 @@ make_eq_member(EquivalenceClass *ec, Expr *expr, Relids
relids,
ec->ec_has_const = true;
/* it can't affect ec_relids */
}
+ em->iterator_generation = 0;
return em;
}
@@ -930,6 +931,7 @@ find_ec_member_matching_expr(EquivalenceClass *ec,
expr = ((RelabelType *) expr)->arg;
setup_eclass_member_iterator(&it, ec, relids);
+ it.warn_if_returned_before = false;
while ((em = eclass_member_iterator_next(&it)) != NULL)
{
Expr *emexpr;
@@ -3149,10 +3151,14 @@ add_setop_child_rel_equivalences(PlannerInfo *root,
RelOptInfo *child_rel,
* ec - The EquivalenceClass from which to iterate members.
* child_relids - The relids to return child members for.
*/
+uint64 iterator_generation = 0;
+
void
setup_eclass_member_iterator(EquivalenceMemberIterator *it,
EquivalenceClass *ec,
Relids child_relids)
{
+ it->iterator_generation = ++iterator_generation;
+ it->warn_if_returned_before = true;
it->ec = ec;
/* no need to set this if the class has no child members */
it->child_relids = ec->ec_childmembers != NULL ? child_relids : NULL;
@@ -3179,6 +3185,9 @@ nextcell:
EquivalenceMember *em;
em = lfirst_node(EquivalenceMember, it->current_cell);
+ if (it->warn_if_returned_before &&
em->iterator_generation == it->iterator_generation)
+ elog(NOTICE, "returned before %llu",
it->iterator_generation);
+ em->iterator_generation = it->iterator_generation;
it->current_cell = lnext(it->current_list,
it->current_cell);
return em;
}
diff --git b/src/include/nodes/pathnodes.h a/src/include/nodes/pathnodes.h
index a44f5bb1f60..60f156da136 100644
--- b/src/include/nodes/pathnodes.h
+++ a/src/include/nodes/pathnodes.h
@@ -1514,6 +1514,7 @@ typedef struct EquivalenceMember
JoinDomain *em_jdomain; /* join domain containing the source
clause */
/* if em_is_child is true, this links to corresponding EM for top
parent */
struct EquivalenceMember *em_parent pg_node_attr(read_write_ignore);
+ uint64 iterator_generation;
} EquivalenceMember;
/*
@@ -1570,6 +1571,8 @@ typedef struct EquivalenceMember
*/
typedef struct
{
+ uint64 iterator_generation;
+ bool warn_if_returned_before;
EquivalenceClass *ec; /* The EquivalenceClass to iterate over
*/
int current_relid; /* Current relid position
within 'relids'. -1
* when still
looping over ec_members and -2
v41-0001-Speedup-child-EquivalenceMember-lookup-in-planne.patch
Description: Binary data
