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>
In-Reply-To: <CAMbWs483a7-8M0pDttG44r-+8Gevn9VG0xNceE3WpkEQxJXPZw@mail.gmail.com>
From: Tender Wang <tndrwang@gmail.com>
Date: Wed, 11 Sep 2024 10:52:05 +0800
Message-ID: <CAHewXNmNN6nu_ZTqESEXjyaXAU2w7DV22P8Vhijg9AJNv0uGLA@mail.gmail.com>
Subject: Re: Eager aggregation, take 3
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>
Content-Type: multipart/alternative; boundary="000000000000efe2880621cf1377"
Archived-At: <https://www.postgresql.org/message-id/CAHewXNmNN6nu_ZTqESEXjyaXAU2w7DV22P8Vhijg9AJNv0uGLA%40mail.gmail.com>
Precedence: bulk

--000000000000efe2880621cf1377
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Richard Guo <guofenglinux@gmail.com> =E4=BA=8E2024=E5=B9=B48=E6=9C=8821=E6=
=97=A5=E5=91=A8=E4=B8=89 15:11=E5=86=99=E9=81=93=EF=BC=9A

> On Fri, Aug 16, 2024 at 4:14=E2=80=AFPM Richard Guo <guofenglinux@gmail.c=
om>
> 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 =E2=80=9Drel_plain" argum=
ent 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 =3D
       make_partial_grouping_target(root, grouped_rel->reltarget,
                                                        extra->havingQual);


--
Thanks,
Tender Wang

--000000000000efe2880621cf1377
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">=
<div dir=3D"ltr" class=3D"gmail_attr">Richard Guo &lt;<a href=3D"mailto:guo=
fenglinux@gmail.com">guofenglinux@gmail.com</a>&gt; =E4=BA=8E2024=E5=B9=B48=
=E6=9C=8821=E6=97=A5=E5=91=A8=E4=B8=89 15:11=E5=86=99=E9=81=93=EF=BC=9A<br>=
</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;b=
order-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Aug 16, 202=
4 at 4:14=E2=80=AFPM Richard Guo &lt;<a href=3D"mailto:guofenglinux@gmail.c=
om" target=3D"_blank">guofenglinux@gmail.com</a>&gt; wrote:<br>
&gt; I had a self-review of this patchset and made some refactoring,<br>
&gt; especially to the function that creates the RelAggInfo structure for a=
<br>
&gt; given relation.=C2=A0 While there were no major changes, the code shou=
ld<br>
&gt; now be simpler.<br>
<br>
I found a bug in v10 patchset: when we generate the GROUP BY clauses<br>
for the partial aggregation that is pushed down to a non-aggregated<br>
relation, we may produce a clause with a tleSortGroupRef that<br>
duplicates one already present in the query&#39;s groupClause, which would<=
br>
cause problems.<br>
<br>
Attached is the updated version of the patchset that fixes this bug<br>
and includes further code refactoring.<br><br>
</blockquote></div><div><br></div>I continue to review the v11 version patc=
hes. Here are some my thoughts.<div><br></div><div>1. In make_one_rel(), we=
 have the below codes:</div><div>/*<br>	 * Build grouped base relations for=
 each base rel if possible.<br>	 */<br>	setup_base_grouped_rels(root);<br><=
/div><div><br></div><div>As far as I know, each base rel only has one group=
ed base relation, if possible.</div><div>The comments may be changed to &qu=
ot;Build a grouped base relation for each base rel if possible.&quot;</div>=
<div><br></div><div>2.=C2=A0 According to the comments of=C2=A0<span style=
=3D"font-size:14px">generate_grouped_paths(), we may generate paths for a g=
rouped </span></div><div><span style=3D"font-size:14px">relation on top of =
paths of join relation. So the =E2=80=9Drel_plain&quot; argument in generat=
e_grouped_paths() may be</span></div><div><span style=3D"font-size:14px">co=
nfused. &quot;plain&quot; usually means &quot;base rel&quot; .  How about R=
e-naming rel_plain to input_rel?</span></div><div><span style=3D"font-size:=
14px"><br></span></div><div><span style=3D"font-size:14px">3.  In </span>cr=
eate_partial_grouping_paths(), The partially_grouped_rel could have been al=
ready created due to eager</div><div>aggregation. If partially_grouped_rel =
exists,=C2=A0 its reltarget has been created. So do we need below logic?</d=
iv><div><br></div><div>/*<br>	 * Build target list for partial aggregate pa=
ths.=C2=A0 These paths cannot just<br>	 * emit the same tlist as regular ag=
gregate paths, because (1) we must<br>	 * include Vars and Aggrefs needed i=
n HAVING, which might not appear in<br>	 * the result tlist, and (2) the Ag=
grefs must be set in partial mode.<br>	 */<br>	partially_grouped_rel-&gt;re=
ltarget =3D<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0make_partial_grouping_target(root=
, grouped_rel-&gt;reltarget,<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 extra-&gt;havingQual);<br></div><div><br></div><div><br></div><div>-=
-</div><div><div dir=3D"ltr" class=3D"gmail_signature"><div dir=3D"ltr"><di=
v>Thanks,</div><div>Tender Wang</div></div></div></div></div>

--000000000000efe2880621cf1377--