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