Re: Removing unneeded self joins

Hi Andrei!

On Fri, Jan 31, 2025 at 6:57 AM Andrei Lepikhov <lepihov(at)gmail(dot)com> wrote:
> On 1/10/25 10:13, Alexander Korotkov wrote:
> > I've got an off-list report from Alexander Lakhin. The failing query
> > is added to the regression tests in the revised patch. The query was
> > failing with an error "negative bitmapset member not allowed" issued
> > in adjust_relid_set(). In order to fix that I've to teach
> > adjust_relid_set() about negative newrelid, it became even more
> > similar to replace_relid(). Notable this error happens in
> > remove_leftjoinrel_from_query(). This seems to be consequence that we
> > made self-join removal and left-joins removal use common
> > infrastructure.
> I have analysed your change. It pertains to the situation where PHV
> references both the outer join and its inner join in phrels but actually
> utilises only the outer side of the join.
>
> I agree with your proposed fix and have just rewritten the query to make
> it simpler and more consistent with the regression tests (see new
> version of the patch in attachment).
>
> Additionally, I noticed that the remove_useless_joins code uses the term
> "join removal" in the join.sql tests. I wonder if it might be more
> appropriate to revert the name from "Self Join Elimination" back to
> "Self Join Removal."

Great, thank you.

Regarding adjust_relid_set() and replace_relid(). I think they are
now strictly equivalent, except for the case then old relid is given
and not found. In this case adjust_relid_set() returns the original
relids while replace_relid() returns a copy. The behavior of
adjust_relid_set() appears more desirable as we don't need extra
copying when no modification is done. So, I've replaced all
replace_relid() with adjust_relid_set().

Also, I did some grammar correction to your new comment in tests.

------
Regards,
Alexander Korotkov
Supabase