MIME-Version: 1.0
References: 
 <CAMbWs48JC4OVqE=3gMB6se2WmRNNfMyFyYxm-09vgpm+Vwe8Hg@mail.gmail.com>
 <7f6439c7-e5a7-4d0c-82c9-4087794cd9d0@gmail.com>
In-Reply-To: <7f6439c7-e5a7-4d0c-82c9-4087794cd9d0@gmail.com>
From: wenhui qiu <qiuwenhuifx@gmail.com>
Date: Tue, 7 Apr 2026 17:57:03 +0800
Message-ID: 
 <CAGjGUAKZ-gaT6z=4_BR8J5Xq7c85ckWuZ3_sHUo6oseto6npHA@mail.gmail.com>
Subject: Re: Clean up remove_rel_from_query() after self-join elimination
 commit
To: Richard Guo <guofenglinux@gmail.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="000000000000dd4975064edbce0d"
Archived-At: 
 <https://www.postgresql.org/message-id/CAGjGUAKZ-gaT6z%3D4_BR8J5Xq7c85ckWuZ3_sHUo6oseto6npHA%40mail.gmail.com>
Precedence: bulk

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

HI Richard
> While working on reusing remove_rel_from_query() for inner-join
> removal, I noticed that the function is in pretty rough shape.  The
> self-join elimination (SJE) commit grafted self-join removal onto
> remove_rel_from_query(), which was originally written for left-join
> removal only.

Thanks for working on this.

I see there are already asserts here for the mode split and for ojrelid
validity. What I had in mind was slightly narrower: making the joinrelids
contract explicit as well.As far as I can tell, the existing asserts would
still allow an outer-join call with joinrelids =3D=3D NULL, even though
joinrelids is later used in the outer-join-specific PHV handling.
I wonder if something like

```c
Assert(!is_outer_join || joinrelids !=3D NULL);
Assert(!is_self_join || joinrelids =3D=3D NULL);


Thanks

On Tue, Apr 7, 2026 at 4:22=E2=80=AFPM Andrei Lepikhov <lepihov@gmail.com> =
wrote:

> On 06/04/2026 10:11, Richard Guo wrote:
> > Thoughts?
> Thanks for your efforts!
>
> The main goal of the SJE feature was to find common ground within the
> community - to come up with a solution that everyone could get behind on
> whether to allow optimisations that address redundant queries and reduce
> query tree complexity in early planning stages. So, some code roughness
> was ok.
>
> I looked through your code, though maybe not as deeply as this part of
> the planner deserves. Overall, it looks good, and I didn=E2=80=99t spot a=
ny
> obvious problems, but I do have a few comments. We added some
> =E2=80=98redundant=E2=80=99 checks with future optimisations in mind, so =
others can rely
> on these functions to remove tails from query structures or to error if
> something was left behind.
>
> You explicitly write:
>
> =E2=80=98Each specific caller remains responsible for updating any remain=
ing
> data structures required by its unique removal logic=E2=80=99
>
> that differs from the initial idea. At the same time, by the end of SJE
> development, I wasn=E2=80=99t so sure we could invent a universal approac=
h to
> guarantee the cleanup of the query tree - mostly because of the inherent
> volatility of the PlannerInfo structure and because we had not agreed to
> make the parse tree a read-only structure.
>
> So, I=E2=80=99m fine with the changes in this patch.
>
> --
> regards, Andrei Lepikhov,
> pgEdge
>
>
>

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

<div dir=3D"ltr">HI=C2=A0Richard<div>&gt; While working on reusing remove_r=
el_from_query() for inner-join<br>&gt; removal, I noticed that the function=
 is in pretty rough shape.=C2=A0 The<br>&gt; self-join elimination (SJE) co=
mmit grafted self-join removal onto<br>&gt; remove_rel_from_query(), which =
was originally written for left-join<br>&gt; removal only.<br></div><div><b=
r></div><div>Thanks for working on this.<br><br>I see there are already ass=
erts here for the mode split and for ojrelid validity. What I had in mind w=
as slightly narrower: making the joinrelids contract explicit as well.As fa=
r as I can tell, the existing asserts would still allow an outer-join call =
with joinrelids =3D=3D NULL, even though joinrelids is later used in the ou=
ter-join-specific PHV handling.<br>I wonder if something like<br><br>```c<b=
r>Assert(!is_outer_join || joinrelids !=3D NULL);<br>Assert(!is_self_join |=
| joinrelids =3D=3D NULL);<br></div><div><br></div><div><br></div><div><br>=
</div><div>Thanks=C2=A0</div></div><br><div class=3D"gmail_quote gmail_quot=
e_container"><div dir=3D"ltr" class=3D"gmail_attr">On Tue, Apr 7, 2026 at 4=
:22=E2=80=AFPM Andrei Lepikhov &lt;<a href=3D"mailto:lepihov@gmail.com">lep=
ihov@gmail.com</a>&gt; wrote:<br></div><blockquote class=3D"gmail_quote" st=
yle=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padd=
ing-left:1ex">On 06/04/2026 10:11, Richard Guo wrote:<br>
&gt; Thoughts?<br>
Thanks for your efforts!<br>
<br>
The main goal of the SJE feature was to find common ground within the <br>
community - to come up with a solution that everyone could get behind on <b=
r>
whether to allow optimisations that address redundant queries and reduce <b=
r>
query tree complexity in early planning stages. So, some code roughness <br=
>
was ok.<br>
<br>
I looked through your code, though maybe not as deeply as this part of <br>
the planner deserves. Overall, it looks good, and I didn=E2=80=99t spot any=
 <br>
obvious problems, but I do have a few comments. We added some <br>
=E2=80=98redundant=E2=80=99 checks with future optimisations in mind, so ot=
hers can rely <br>
on these functions to remove tails from query structures or to error if <br=
>
something was left behind.<br>
<br>
You explicitly write:<br>
<br>
=E2=80=98Each specific caller remains responsible for updating any remainin=
g <br>
data structures required by its unique removal logic=E2=80=99<br>
<br>
that differs from the initial idea. At the same time, by the end of SJE <br=
>
development, I wasn=E2=80=99t so sure we could invent a universal approach =
to <br>
guarantee the cleanup of the query tree - mostly because of the inherent <b=
r>
volatility of the PlannerInfo structure and because we had not agreed to <b=
r>
make the parse tree a read-only structure.<br>
<br>
So, I=E2=80=99m fine with the changes in this patch.<br>
<br>
-- <br>
regards, Andrei Lepikhov,<br>
pgEdge<br>
<br>
<br>
</blockquote></div>

--000000000000dd4975064edbce0d--