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: Thu, 5 Sep 2024 09:40:18 +0800
Message-ID: <CAHewXN=tGYzuuHMCR_0dQWZX-oV-gJsSJnAkt-q2n6yRzFm7ww@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="00000000000033b55b0621556026"
Archived-At: <https://www.postgresql.org/message-id/CAHewXN%3DtGYzuuHMCR_0dQWZX-oV-gJsSJnAkt-q2n6yRzFm7ww%40mail.gmail.com>
Precedence: bulk

--00000000000033b55b0621556026
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 review the v11 patch set, and here are a few of my thoughts:

1.  in setup_eager_aggregation(), before calling create_agg_clause_infos(),
it does
some checks if eager aggregation is available. Can we move those checks
into a function,
for example, can_eager_agg(), like can_partial_agg() does?

2.  I found that outside of joinrel.c we all use IS_DUMMY_REL,  but in
joinrel.c, Tom always uses
is_dummy_rel(). Other commiters use IS_DUMMY_REL.

3.  The attached patch does not consider FDW when creating a path for
grouped_rel or grouped_join.
Do we need to think about FDW?

I haven't finished reviewing the patch set. I will continue to learn this
feature.

--=20
Thanks,
Tender Wang

--00000000000033b55b0621556026
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.</blockquote><div><br></div><div>=C2=
=A0I review the v11 patch set, and here are a few of my thoughts:</div><div=
>=C2=A0</div></div><div>1.=C2=A0 in setup_eager_aggregation(), before calli=
ng create_agg_clause_infos(), it does</div><div>some checks if eager aggreg=
ation is available. Can we move those checks into a function,</div><div>for=
 example, can_eager_agg(), like can_partial_agg() does?</div><div><br></div=
><div>2.=C2=A0 I found that outside of joinrel.c we all use IS_DUMMY_REL,=
=C2=A0 but in joinrel.c, Tom always uses</div><div>is_dummy_rel(). Other co=
mmiters use IS_DUMMY_REL.=C2=A0</div><div><br></div><div>3.=C2=A0 The attac=
hed patch does not consider FDW when creating a path for grouped_rel or gro=
uped_join.</div><div>Do we need to think about FDW?</div><div><br></div><di=
v>I haven&#39;t finished reviewing the patch set. I will continue to learn =
this feature.</div><div><br></div><span class=3D"gmail_signature_prefix">--=
 </span><br><div dir=3D"ltr" class=3D"gmail_signature"><div dir=3D"ltr"><di=
v>Thanks,</div><div>Tender Wang</div></div></div></div>

--00000000000033b55b0621556026--