public inbox for [email protected]  

help / color / mirror / Atom feed

From: wenhui qiu <[email protected]>
To: Richard Guo <[email protected]>
Cc: Pg Hackers <[email protected]>
Subject: Re: Clean up remove_rel_from_query() after self-join elimination commit
Date: Tue, 7 Apr 2026 17:57:03 +0800
Message-ID: <CAGjGUAKZ-gaT6z=4_BR8J5Xq7c85ckWuZ3_sHUo6oseto6npHA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAMbWs48JC4OVqE=3gMB6se2WmRNNfMyFyYxm-09vgpm+Vwe8Hg@mail.gmail.com>
	<[email protected]>

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 == 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 != NULL);
Assert(!is_self_join || joinrelids == NULL);



Thanks

On Tue, Apr 7, 2026 at 4:22 PM Andrei Lepikhov <[email protected]> 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’t spot any
> obvious problems, but I do have a few comments. We added some
> ‘redundant’ 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:
>
> ‘Each specific caller remains responsible for updating any remaining
> data structures required by its unique removal logic’
>
> that differs from the initial idea. At the same time, by the end of SJE
> development, I wasn’t so sure we could invent a universal approach 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’m fine with the changes in this patch.
>
> --
> regards, Andrei Lepikhov,
> pgEdge
>
>
>

view thread (7+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Clean up remove_rel_from_query() after self-join elimination commit
  In-Reply-To: <CAGjGUAKZ-gaT6z=4_BR8J5Xq7c85ckWuZ3_sHUo6oseto6npHA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox