Richard Guo <guofengli...@gmail.com> 于2024年8月21日周三 15:11写道：
> On Fri, Aug 16, 2024 at 4:14 PM Richard Guo <guofengli...@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. -- Thanks, Tender Wang
