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 > > >
