MIME-Version: 1.0
References: <ZQhYYRhUxpW3PSf9@telsasoft.com> <CAMbWs48X+cK2pEaZDVVjd+nOJzb1dN1CO1OKz+6D0FX2OTCGbg@mail.gmail.com>
 <CAApHDvpJJigQRW29TppTOPYp+Aui0mtd3MpfRxyKv=N-tB62jQ@mail.gmail.com>
 <CAApHDvo7RzcQYw-gnkZr6QCijCqf8vJLkJ4XFk-KawvyAw109Q@mail.gmail.com> <CAApHDvqoMf_kxQKON+FLQh8DbMKe62R=OdrZoenb6xDYWmQVUQ@mail.gmail.com>
In-Reply-To: <CAApHDvqoMf_kxQKON+FLQh8DbMKe62R=OdrZoenb6xDYWmQVUQ@mail.gmail.com>
From: Richard Guo <guofenglinux@gmail.com>
Date: Sun, 8 Oct 2023 18:52:38 +0800
Message-ID: <CAMbWs48VKgJyqfDNtKsYnNsPYWqA=stPKenehVz1NN0HTFV9mg@mail.gmail.com>
Subject: Re: pg16: XX000: could not find pathkey item to sort
To: David Rowley <dgrowleyml@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="0000000000004a9c9d06073246b0"
Archived-At: <https://www.postgresql.org/message-id/CAMbWs48VKgJyqfDNtKsYnNsPYWqA%3DstPKenehVz1NN0HTFV9mg%40mail.gmail.com>
Precedence: bulk

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

On Thu, Oct 5, 2023 at 2:26=E2=80=AFPM David Rowley <dgrowleyml@gmail.com> =
wrote:

> So in short, I propose the attached fix without any regression tests
> because I feel that any regression test would just mark that there was
> a big in create_agg_path() and not really help with ensuring we don't
> end up with some similar problem in the future.


If the pathkeys that were added by adjust_group_pathkeys_for_groupagg()
are computable from the targetlist, it seems that we do not need to trim
them off, because prepare_sort_from_pathkeys() will add resjunk target
entries for them.  But it's also no harm if we trim them off.  So I
think the patch is a pretty safe fix.  +1 to it.


> I have some concerns that the assert_pathkeys_in_target() function
> might be a little heavyweight for USE_ASSERT_CHECKING builds. So I'm
> not proposing to commit that without further discussion.


Yeah, it looks like some heavy to call assert_pathkeys_in_target() for
each path node.  Can we run some benchmarks to see how much overhead it
would bring to USE_ASSERT_CHECKING build?

Thanks
Richard

--0000000000004a9c9d06073246b0
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, Oct 5, 2023 at 2:26=E2=80=AFPM 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">
So in short, I propose the attached fix without any regression tests<br>
because I feel that any regression test would just mark that there was<br>
a big in create_agg_path() and not really help with ensuring we don&#39;t<b=
r>
end up with some similar problem in the future.</blockquote><div><br></div>=
<div>If the pathkeys that were added by adjust_group_pathkeys_for_groupagg(=
)<br>are computable from the targetlist, it seems that we do not need to tr=
im<br>them off, because prepare_sort_from_pathkeys() will add resjunk targe=
t<br>entries for them.=C2=A0 But it&#39;s also no harm if we trim them off.=
=C2=A0 So I<br>think the patch is a pretty safe fix. =C2=A0+1 to it.<br></d=
iv><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0=
px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I have some concerns that the assert_pathkeys_in_target() function<br>
might be a little heavyweight for USE_ASSERT_CHECKING builds. So I&#39;m<br=
>
not proposing to commit that without further discussion.</blockquote><div><=
br></div><div>Yeah, it looks like some heavy to call assert_pathkeys_in_tar=
get() for<br>each path node.=C2=A0 Can we run some benchmarks to see how mu=
ch overhead it<br>would bring to USE_ASSERT_CHECKING build?<br><br>Thanks<b=
r>Richard<br></div></div></div>

--0000000000004a9c9d06073246b0--