Horton

Re: Eager aggregation, take 3

Richard Guo <guofenglinux@gmail.com>

View thread View on postgr.es
From: Richard Guo <guofenglinux@gmail.com>
To: Tender Wang <tndrwang@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-25T07:02:51Z
Lists: pgsql-hackers 
On Wed, Sep 11, 2024 at 10:52 AM Tender Wang <tndrwang@gmail.com> wrote:
> 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."

Yeah, each base rel has only one grouped rel.  However, there is a
comment nearby stating 'consider_parallel flags for each base rel',
which confuses me about whether it should be singular or plural in
this context.  Perhaps someone more proficient in English could
clarify this.

> 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?

I don't think 'plain relation' necessarily means 'base relation'.  In
this context I think it can mean 'non-grouped relation'.  But maybe
I'm wrong.

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

Yeah, maybe we can avoid building the target list here for
partially_grouped_rel that is generated by eager aggregation.

Thanks
Richard

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