Re: Removing unneeded self joins

On Thu, Jul 4, 2024 at 11:40 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> On Thu, Jul 4, 2024 at 11:04 AM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
> >
> > On Thu, Jul 4, 2024 at 5:15 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> > > in remove_self_join_rel, i have
> > > ```ChangeVarNodes((Node *) root->parse, toRemove->relid, toKeep->relid, 0);```
> > > which will change the joinlist(RangeTblRef) from (1,2) to (2,2).
> > > Immediately after this call, I wrote a function (restore_rangetblref)
> > > to restore the joinlist as original (1,2).
> > > then remove_rel_from_joinlist won't error out.
> > > see remove_self_join_rel, restore_rangetblref.
> >
> > Thank you, now this is clear. Could we add additional parameters to
> > ChangeVarNodes() instead of adding a new function which reverts part
> > of changes.
> >
>
> I didn't dare to. we have 42 occurrences of ChangeVarNodes.
> adding a parameter to it only for one location seems not intuitive.
>
> Now I have tried.
> changing to
> `ChangeVarNodes(Node *node, int rt_index, int new_index, int
> sublevels_up, bool change_RangeTblRef)`
>
> /* Replace varno in all the query structures */
> ChangeVarNodes((Node *) root->parse, toRemove->relid, toKeep->relid, 0, false);
> ```
>
> it seems to work, pass the regression test.
> ```ChangeVarNodes((Node *) root->parse, toRemove->relid,
> toKeep->relid, 0, false);```
> is in remove_self_join_rel, remove_self_joins_one_group,
> remove_self_joins_recurse.
> all other places are ```ChangeVarNodes((Node *) root->parse,
> toRemove->relid, toKeep->relid, 0, true);```
> so ChangeVarNodes add a parameter will only influence the SJE feature.

Good. But I think it's not necessary to to replace function signature
in all the 42 occurrences. This will make our patch unnecessarily
conflict with others. Instead we can have two functions
ChangeVarNodes(original function signature) and
ChangeVarNodesExtended(extended function signature). Then existing
occurrences can still use ChangeVarNodes(), which will be just
shortcut for ChangeVarNodesExtended().

------
Regards,
Alexander Korotkov
Supabase