MIME-Version: 1.0
References: <CAApHDvo2ji+hdxrxfXtRtsfSVw3to2o1nCO20qimw0dUGK8hcQ@mail.gmail.com>
 <CAMbWs49jxZkfSP-9rG0Go4W7=pXCgS7jcjYJEERGJF3SoOFAqg@mail.gmail.com> <CAApHDvpc=Gu4J47iSzRENGfbxDeYnBhDqzs4_+Z+3LE3iswq0Q@mail.gmail.com>
In-Reply-To: <CAApHDvpc=Gu4J47iSzRENGfbxDeYnBhDqzs4_+Z+3LE3iswq0Q@mail.gmail.com>
From: Richard Guo <guofenglinux@gmail.com>
Date: Fri, 9 Jun 2023 16:57:02 +0800
Message-ID: <CAMbWs4_drpKVckSys8-pZe8OcFSPgoRZHseJ4KP2tfVPePYTog@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="00000000000016fddc05fdae8e6b"
Archived-At: <https://www.postgresql.org/message-id/CAMbWs4_drpKVckSys8-pZe8OcFSPgoRZHseJ4KP2tfVPePYTog%40mail.gmail.com>
Precedence: bulk

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

On Fri, Jun 9, 2023 at 8:13=E2=80=AFAM David Rowley <dgrowleyml@gmail.com> =
wrote:

> After looking again at nodeWindowAgg.c, I think it might be possible
> to do a bit more work and apply this to ORDER BY items too.  Without
> an ORDER BY clause, all rows in the partition are peers of each other,
> and if the ORDER BY column is redundant due to belonging to a
> redundant pathkey, then those rows must also be peers too since the
> redundant pathkey must mean all rows have an equal value in the
> redundant column.
>
> However, there is a case where we must be much more careful.  The
> comment you highlighted in create_windowagg_plan() does mention this.
> It reads "we must *not* remove the ordering column for RANGE OFFSET
> cases".


I see.  I tried to run the query below

select a, b, sum(a) over (order by b range between 10 preceding and current
row) from t where b =3D 2;
server closed the connection unexpectedly

and if we've removed redundant items in wc->orderClause the query would
trigger the Assert in update_frameheadpos().

            /* We must have an ordering column */
            Assert(node->ordNumCols =3D=3D 1);


>
> It might be possible to make adjustments in nodeWindowAgg.c to have
> the equality checks come out as true when there is no ORDER BY.
> update_frameheadpos() is one location that would need to be adjusted.
> It would need further study to ensure we don't accidentally break
> anything. I've not done that study, so won't be adjusting the patch
> for now.


I'm also not sure if doing that is safe in all cases.  Hmm, do you think
we can instead check wc->frameOptions to see if it is the RANGE OFFSET
case in make_pathkeys_for_window(), and decide to not remove or remove
redundant ORDER BY items according to whether it is or not RANGE OFFSET?

Thanks
Richard

--00000000000016fddc05fdae8e6b
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 Fri, Jun 9, 2023 at 8:13=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">
After looking again at nodeWindowAgg.c, I think it might be possible<br>
to do a bit more work and apply this to ORDER BY items too.=C2=A0 Without<b=
r>
an ORDER BY clause, all rows in the partition are peers of each other,<br>
and if the ORDER BY column is redundant due to belonging to a<br>
redundant pathkey, then those rows must also be peers too since the<br>
redundant pathkey must mean all rows have an equal value in the<br>
redundant column.<br>
<br>
However, there is a case where we must be much more careful.=C2=A0 The<br>
comment you highlighted in create_windowagg_plan() does mention this.<br>
It reads &quot;we must *not* remove the ordering column for RANGE OFFSET<br=
>
cases&quot;.</blockquote><div><br></div><div>I see.=C2=A0 I tried to run th=
e query below<br><br>select a, b, sum(a) over (order by b range between 10 =
preceding and current row) from t where b =3D 2;<br>server closed the conne=
ction unexpectedly<br><br>and if we&#39;ve removed redundant items in wc-&g=
t;orderClause the query would<br>trigger the Assert in update_frameheadpos(=
).<br><br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* We must have an orde=
ring column */<br>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 Assert(node-&gt=
;ordNumCols =3D=3D 1);<br></div><div>=C2=A0</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">
<br>
It might be possible to make adjustments in nodeWindowAgg.c to have<br>
the equality checks come out as true when there is no ORDER BY.<br>
update_frameheadpos() is one location that would need to be adjusted.<br>
It would need further study to ensure we don&#39;t accidentally break<br>
anything. I&#39;ve not done that study, so won&#39;t be adjusting the patch=
<br>
for now.</blockquote><div><br></div><div>I&#39;m also not sure if doing tha=
t is safe in all cases.=C2=A0 Hmm, do you think<br>we can instead check wc-=
&gt;frameOptions to see if it is the RANGE OFFSET<br>case in make_pathkeys_=
for_window(), and decide to not remove or remove<br>redundant ORDER BY item=
s according to whether it is or not RANGE OFFSET?<br><br>Thanks<br>Richard<=
br></div></div></div>

--00000000000016fddc05fdae8e6b--