On 4/2/25 15:26, Richard Guo wrote:
> While working on another patch, I looked at ChangeVarNodes() and found
> some issues introduced by the self-join elimination commit. I think
> it'd better to fix or improve them.
>
> * ChangeVarNodes_walker includes special handling for RestrictInfo
> nodes. And it adjusts RestrictInfo.orclause only if the rt_index of
> the relation to be removed is contained in RestrictInfo.clause_relids.
>
> I don't think this is correct. Even if the to-be-removed relation is
> not present in clause_relids, it can still be found somewhere within
> the orclause. As an example, consider:
Yeah, discovering how we tolerated such a gaffe I found that the comment
on the clause_relids field is quite vague here:
"The relids (varnos+varnullingrels) actually referenced in the clause:"
and only in the RestrictInfo description you can find some additional
information. I think we should modify the check, uniting clause_relids
and required_relids before searching for the removing relid.
> + rinfo->num_base_rels = bms_num_members(rinfo->clause_relids);
>
> I don't think this is correct either. num_base_rels is the number of
> base rels in clause_relids and should not count outer-join relids.
> And we have no guarantee that clause_relids does not include any
> outer-join relids.
It is a clear mistake, no apologies to me.
>
> To fix, I think we should exclude all outer-join relids. Perhaps we
> can leverage root->outer_join_rels to achieve this.
I think we may use a difference in size of rinfo->clause_relids before
and after adjustion. If something were removed, just decrease num_base_rels.
> * Speaking of comments, I’ve noticed that some changes are lacking
> comments. For example, there are almost no comments regarding the
> special handling of RestrictInfo nodes in ChangeVarNodes_walker,
> except for an explanation about adding the NullTest.
Ok, it seems that comments were removed too hastily. Not addressed it yet.
I also added little code refactoring to make it more readable. See
fix.diff in attachment as my proposal for future discussion.
--
regards, Andrei Lepikhov
