MIME-Version: 1.0
References: <CAMbWs48jzLrPt1J_00ZcPZXWUQKawQOFE8ROc-ADiYqsqrpBNw@mail.gmail.com>
 <87il22cj51.fsf@163.com> <CAMbWs49=eAd2W9jCtGhaZPPp+SOC_2rg16RTG74xAht=hkr5JQ@mail.gmail.com>
 <CAMbWs49Nc4M3H+eCf1+8w8piDyEECjRb-gK_JMF4VvcyWwGEVQ@mail.gmail.com>
 <CAMbWs49E_dR0nobsExsyetpnBpHObLTsQLsEbWKQLkh0omPxNg@mail.gmail.com>
 <CAMbWs49B_qUiHvu2EqLHZRpLr3p_+QPBs50n2=L5ibYzniwTzA@mail.gmail.com>
 <CAMbWs48KCQtDymnYi4M=Vz+WMzo3fkBxffJsyk6VX6hOXXv+VA@mail.gmail.com>
 <CAMbWs49sv_MuOYqqrtmBN_oYf8VSQ2BXDwXaTpJTn_YfwyYdWQ@mail.gmail.com>
 <CAMbWs49U8Sddx_fGszPdvA3jp_nheynxaqm5Y4NqMV21VBYAuQ@mail.gmail.com>
 <CAMbWs4-LwyOg9ga+NVF7yQbMi0ZsZdN1G_sO2v=YJHV18=19+A@mail.gmail.com>
 <CALA8mJquG_zCJXfVwash5LKqHGtZXQmq7RfTSaRDUzGYeW=7Rw@mail.gmail.com>
 <CAMbWs4_EjgcBib5+y1LYcGB3EK3Y6R+OOxGKfJo42fDovadk1g@mail.gmail.com>
 <CALA8mJqe0anNM8_V6cOeOQnCHUTQggn7iOQNyQr1VaN_xMjz+w@mail.gmail.com>
 <CAMbWs48eE-s-jCicC8pSVfXk8Ws-ZvUKnsw8qH-DkVBdYv0eJQ@mail.gmail.com>
 <CAMbWs483a7-8M0pDttG44r-+8Gevn9VG0xNceE3WpkEQxJXPZw@mail.gmail.com> <CAHewXNmNN6nu_ZTqESEXjyaXAU2w7DV22P8Vhijg9AJNv0uGLA@mail.gmail.com>
In-Reply-To: <CAHewXNmNN6nu_ZTqESEXjyaXAU2w7DV22P8Vhijg9AJNv0uGLA@mail.gmail.com>
From: Richard Guo <guofenglinux@gmail.com>
Date: Wed, 25 Sep 2024 15:02:51 +0800
Message-ID: <CAMbWs49r+6WBszieGpEGnhacD+yo_rU85m7NZ=TFu3oiNHmvmw@mail.gmail.com>
Subject: Re: Eager aggregation, take 3
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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAMbWs49r%2B6WBszieGpEGnhacD%2Byo_rU85m7NZ%3DTFu3oiNHmvmw%40mail.gmail.com>
Precedence: bulk

On Wed, Sep 11, 2024 at 10:52=E2=80=AFAM Tender Wang <tndrwang@gmail.com> w=
rote:
> 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 po=
ssible.
> The comments may be changed to "Build a grouped base relation for each ba=
se 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 generat=
e paths for a grouped
> relation on top of paths of join relation. So the =E2=80=9Drel_plain" arg=
ument in generate_grouped_paths() may be
> confused. "plain" usually means "base rel" . How about Re-naming rel_plai=
n 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 ha=
ve been already created due to eager
> aggregation. If partially_grouped_rel exists,  its reltarget has been cre=
ated. 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 =3D
>        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