Re: Eager aggregation, take 3

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".

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

--
Robert Haas
EDB: http://www.enterprisedb.com