Horton

Re: Eager aggregation, take 3

Tender Wang <tndrwang@gmail.com>

View thread View on postgr.es
From: Tender Wang <tndrwang@gmail.com>
To: Richard Guo <guofenglinux@gmail.com>
Cc: Paul George <p.a.george19@gmail.com>, Andy Fan <zhihuifan1213@163.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>, pgsql-hackers@lists.postgresql.org, Robert Haas <robertmhaas@gmail.com>
Date: 2024-09-11T02:52:05Z
Lists: pgsql-hackers 
Richard Guo <guofenglinux@gmail.com> 于2024年8月21日周三 15:11写道：

> 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);


--
Thanks,
Tender Wang

Commits

  1. Fix eager aggregation for semi/antijoin inner rels

  2. Cover additional errors and corner conditions in repack.c

  3. Fix volatile function evaluation in eager aggregation

  4. Fix collation handling for grouping keys in eager aggregation

  5. Rename apply_at to apply_agg_at for clarity

  6. Fix comment in eager_aggregate.sql

  7. Remove unnecessary include of "utils/fmgroids.h"

  8. Implement Eager Aggregation

  9. Allow negative aggtransspace to indicate unbounded state size

  10. Add macros for looping through a List without a ListCell.

  11. Account for the effect of lossy pages when costing bitmap scans.

  12. Fix a thinko in join_is_legal: when we decide we can implement a semijoin