Re: Eager aggregation, take 3

On Tue, Oct 29, 2024 at 9:59 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Wed, Sep 25, 2024 at 3:03 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> > On Wed, Sep 11, 2024 at 10:52 AM Tender Wang <tndrwang(at)gmail(dot)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.
>
> It's not confusing the way you have it, but I think an English teacher
> wouldn't like it, because part of the sentence is singular ("each base
> rel") and the other part is plural ("grouped base relations").
> Tender's proposed rewrite fixes that. Another way to fix it is to
> write "Build group relations for base rels where possible".

Thank you for the suggestion. The new wording looks much better
grammatically. It seems to me that we should address the nearby
comment too, which goes like "consider_parallel flags for each base
rel", as each rel has only one consider_parallel flag.

> > > 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.
>
> We use the term "plain relation" in several different ways. In the
> header comments for addFkRecurseReferenced, it means a non-partitioned
> relation. In the struct comments for RangeTblEntry, it means any sort
> of named thing in pg_class that you can scan, so either a partitioned
> or unpartitioned table but not a join or a table function or
> something. AFAICT, the most common meaning of "plain relation" is a
> pg_class entry where relkind==RELKIND_RELATION.

Agreed.

Thanks
Richard