MIME-Version: 1.0
References: <CAApHDvo2ji+hdxrxfXtRtsfSVw3to2o1nCO20qimw0dUGK8hcQ@mail.gmail.com>
In-Reply-To: <CAApHDvo2ji+hdxrxfXtRtsfSVw3to2o1nCO20qimw0dUGK8hcQ@mail.gmail.com>
From: Richard Guo <guofenglinux@gmail.com>
Date: Thu, 8 Jun 2023 17:11:17 +0800
Message-ID: <CAMbWs49jxZkfSP-9rG0Go4W7=pXCgS7jcjYJEERGJF3SoOFAqg@mail.gmail.com>
Subject: Re: Remove WindowClause PARTITION BY items belonging to redundant pathkeys
To: David Rowley <dgrowleyml@gmail.com>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="0000000000003a9fcc05fd9aa324"
Archived-At: <https://www.postgresql.org/message-id/CAMbWs49jxZkfSP-9rG0Go4W7%3DpXCgS7jcjYJEERGJF3SoOFAqg%40mail.gmail.com>
Precedence: bulk

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

On Thu, Jun 8, 2023 at 7:37=E2=80=AFAM David Rowley <dgrowleyml@gmail.com> =
wrote:

> What the attached patch does is process each WindowClause and removes
> any items from the PARTITION BY clause that are columns or expressions
> relating to redundant PathKeys.
>
> Effectively, this allows the nodeWindowAgg.c code which stops
> processing WindowAgg rows when the run condition is met to work as the
> PARTITION BY clause is completely removed in the case of the above
> query.  Removing the redundant PARTITION BY items also has the added
> benefit of not having to needlessly check if the next row belongs to
> the same partition as the last row. For the above, that check is a
> waste of time as all rows have relkind =3D 'r'


This is a nice optimization.  I reviewed it and here are my findings.

In create_windowagg_plan there is such comment that says

* ... Note: in principle, it's possible
* to drop some of the sort columns, if they were proved redundant by
* pathkey logic.  However, it doesn't seem worth going out of our way to
* optimize such cases.

Since this patch removes any clauses from the wc->partitionClause for
redundant pathkeys, this comment seems outdated, at least for the sort
columns in partitionClause.

Also I'm wondering if we can do the same optimization to
wc->orderClause.  I tested it with the query below and saw performance
gains.

create table t (a int, b int);
insert into t select 1,2 from generate_series(1,100000)i;
analyze t;

explain analyze
select * from
 (select a, b, rank() over (PARTITION BY a order by b) rank
    from t where b =3D 2)
where a =3D 1 and rank <=3D 10;

With and without this optimization to wc->orderClause the execution time
is 67.279 ms VS. 119.120 ms (both best of 3).

I notice you comment in the patch that doing this is unsafe because it
would change the semantics of peer rows during execution.  Would you
please elaborate on that?

Thanks
Richard

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

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><div class=3D"gmail_quote"><div=
 dir=3D"ltr" class=3D"gmail_attr">On Thu, Jun 8, 2023 at 7:37=E2=80=AFAM Da=
vid Rowley &lt;<a href=3D"mailto:dgrowleyml@gmail.com">dgrowleyml@gmail.com=
</a>&gt; wrote:</div><blockquote class=3D"gmail_quote" style=3D"margin:0px =
0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
What the attached patch does is process each WindowClause and removes<br>
any items from the PARTITION BY clause that are columns or expressions<br>
relating to redundant PathKeys.<br>
<br>
Effectively, this allows the nodeWindowAgg.c code which stops<br>
processing WindowAgg rows when the run condition is met to work as the<br>
PARTITION BY clause is completely removed in the case of the above<br>
query.=C2=A0 Removing the redundant PARTITION BY items also has the added<b=
r>
benefit of not having to needlessly check if the next row belongs to<br>
the same partition as the last row. For the above, that check is a<br>
waste of time as all rows have relkind =3D &#39;r&#39;</blockquote><div><br=
></div><div>This is a nice optimization.=C2=A0 I reviewed it and here are m=
y findings.<br><br>In create_windowagg_plan there is such comment that says=
<br><br>* ... Note: in principle, it&#39;s possible<br>* to drop some of th=
e sort columns, if they were proved redundant by<br>* pathkey logic.=C2=A0 =
However, it doesn&#39;t seem worth going out of our way to<br>* optimize su=
ch cases.<br><br>Since this patch removes any clauses from the wc-&gt;parti=
tionClause for<br>redundant pathkeys, this comment seems outdated, at least=
 for the sort<br>columns in partitionClause.<br><br>Also I&#39;m wondering =
if we can do the same optimization to<br>wc-&gt;orderClause.=C2=A0 I tested=
 it with the query below and saw performance<br>gains.<br><br>create table =
t (a int, b int);<br>insert into t select 1,2 from generate_series(1,100000=
)i;<br>analyze t;<br><br>explain analyze<br>select * from<br>=C2=A0(select =
a, b, rank() over (PARTITION BY a order by b) rank<br>=C2=A0 =C2=A0 from t =
where b =3D 2)<br>where a =3D 1 and rank &lt;=3D 10;<br><br>With and withou=
t this optimization to wc-&gt;orderClause the execution time<br>is 67.279 m=
s VS. 119.120 ms (both best of 3).<br><br>I notice you comment in the patch=
 that doing this is unsafe because it<br>would change the semantics of peer=
 rows during execution.=C2=A0 Would you<br>please elaborate on that?<br><br=
>Thanks<br>Richard<br></div></div></div>

--0000000000003a9fcc05fd9aa324--