On Fri, Aug 16, 2024 at 4:14 PM Richard Guo <guofenglinux@gmail.com> wrote:
> I had a self-review of this patchset and made some refactoring,
> especially to the function that creates the RelAggInfo structure for a
> given relation. While there were no major changes, the code should
> now be simpler.
I found a bug in v10 patchset: when we generate the GROUP BY clauses
for the partial aggregation that is pushed down to a non-aggregated
relation, we may produce a clause with a tleSortGroupRef that
duplicates one already present in the query's groupClause, which would
cause problems.
Attached is the updated version of the patchset that fixes this bug
and includes further code refactoring.
I review the v11 patch set, and here are a few of my thoughts:
1. in setup_eager_aggregation(), before calling create_agg_clause_infos(), it does
some checks if eager aggregation is available. Can we move those checks into a function,
for example, can_eager_agg(), like can_partial_agg() does?
2. I found that outside of joinrel.c we all use IS_DUMMY_REL, but in joinrel.c, Tom always uses
is_dummy_rel(). Other commiters use IS_DUMMY_REL.
3. The attached patch does not consider FDW when creating a path for grouped_rel or grouped_join.
Do we need to think about FDW?
I haven't finished reviewing the patch set. I will continue to learn this feature.
--
