On Tue, 12 Mar 2024 at 23:21, David Rowley <dgrowleyml@gmail.com> wrote:
> I've attached v3.
I spent quite a bit more time looking at this.
I discovered that the dNumGroups wasn't being set as it should have
been for INTERSECT and EXCEPT queries. There was a plan change as a
result of this. I've fixed this by adjusting where dNumGroups is set.
It must be delayed until after the setop child paths have been
created.
Aside from this, the changes I made were mostly cosmetic. However, I
did notice that I wasn't setting the union child RelOptInfo's
ec_indexes in add_setop_child_rel_equivalences(). I also discovered
that we're not doing that properly for the top-level RelOptInfo for
the UNION query prior to this change. The reason is that due to the
Var.varno==0 for the top-level UNION targetlist. The code in
get_eclass_for_sort_expr() effectively misses this relation due to
"while ((i = bms_next_member(newec->ec_relids, i)) > 0)". This
happens to be good because there is no root->simple_rel_array[0]
entry, so happens to prevent that code crashing. It seems ok that
the ec_indexes are not set for the top-level set RelOptInfo as
get_eclass_for_sort_expr() does not make use of ec_indexes while
searching for an existing EquivalenceClass. Maybe we should fix this
varno == 0 hack and adjust get_eclass_for_sort_expr() so that it makes
use of the ec_indexes.
It's possible to see this happen with a query such as:
SELECT oid FROM pg_class UNION SELECT oid FROM pg_class ORDER BY oid;
I didn't see that as a reason not to push this patch as this occurs
both with and without this change, so I've now pushed this patch.
Thank you and Andy for reviewing this.
David