Re: Removing unneeded self joins

On 7/2/24 07:25, jian he wrote:
> to make sure it's correct, I have added a lot of tests,
> Some of this may be contrived, maybe some of the tests are redundant.
Thanks for your job!
I passed through the patches and have some notes:
1. Patch 0001 has not been applied anymore since the previous week's 
changes in the core. Also, there is one place with trailing whitespace.

Looking into the 0002 and 0003 patches, I think they 1) should be merged 
and 2) It makes sense to use the already existing pull_varnos_of_level 
routine instead of a new walker. See the patches in the attachment as a 
sketch.
Also, I'm not sure about the tests. It looks like we have a lot of new 
tests.

However, the main issue mentioned above is the correctness of relid 
replacement in planner structures.
We have the machinery to check and replace relids in a Query. But 
PlannerInfo is a bin for different stuff that the optimisation needs to 
convert the parse tree to the corresponding cloud of paths.
A good demo of the problem is the introduction of the JoinDomain structure:
It contains a relids field and has no tests for that. We haven't known 
for a long time about the issue of SJE not replacing the relid in this 
structure.
The approach with 'Relation Alias' mentioned by Alexander raises many 
hard questions about accessing simple_rel_array directly or, much worse, 
about checking the scope of some clause where we didn't touch 
RelOptInfo, just compare two relids fields.
The safest decision would be to restart query planning over parse tree 
with removed self-joins, but now planner code isn't ready for that yet.
But maybe we should put this problem on the shoulders of a developer and 
made  something like with nodes: perl script which will generate walker 
switch over PlannerInfo structure?

-- 
regards, Andrei Lepikhov