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 continue to review the v11 version patches. Here are some my thoughts.
1. In make_one_rel(), we have the below codes:
/*
* Build grouped base relations for each base rel if possible.
*/
setup_base_grouped_rels(root);
As far as I know, each base rel only has one grouped base relation, if possible.
The comments may be changed to "Build a grouped base relation for each base rel if possible."
2. According to the comments of generate_grouped_paths(), we may generate paths for a grouped
relation on top of paths of join relation. So the ”rel_plain" argument in generate_grouped_paths() may be
confused. "plain" usually means "base rel" . How about Re-naming rel_plain to input_rel?
3. In create_partial_grouping_paths(), The partially_grouped_rel could have been already created due to eager
aggregation. If partially_grouped_rel exists, its reltarget has been created. So do we need below logic?
/*
* Build target list for partial aggregate paths. These paths cannot just
* emit the same tlist as regular aggregate paths, because (1) we must
* include Vars and Aggrefs needed in HAVING, which might not appear in
* the result tlist, and (2) the Aggrefs must be set in partial mode.
*/
partially_grouped_rel->reltarget =
make_partial_grouping_target(root, grouped_rel->reltarget,
extra->havingQual);
--
