Re: Removing unneeded self joins

Hi.

Em sáb., 15 de fev. de 2025 às 06:20, Alexander Korotkov <
aekorotkov@gmail.com> escreveu:

> On Thu, Feb 13, 2025 at 1:00 AM Alexander Korotkov <aekorotkov@gmail.com>
> wrote:
> >
> > On Tue, Feb 11, 2025 at 5:31 PM Alena Rybakina
> > <a.rybakina@postgrespro.ru> wrote:
> > > Hi! Thank you for the work with this subject, I think it is really
> > > important.
> > >
> > > On 10.02.2025 22:58, Alexander Korotkov wrote:
> > > > Hi!
> > > >
> > > > On Mon, Feb 10, 2025 at 7:19 AM Andrei Lepikhov <lepihov@gmail.com>
> wrote:
> > > >> On 9/2/2025 18:41, Alexander Korotkov wrote:
> > > >>> 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().
> > > >> Ok, I glanced into it, and it makes sense to merge these routines.
> > > >> I think the comment to adjust_relid_set() should be arranged, too.
> See
> > > >> the attachment for a short variant of such modification.
> > > >>> Also, I did some grammar correction to your new comment in tests.
> > > >> Thanks!
> > > > I've further revised adjust_relid_set() header comment.
> > > >
> > > > Looking back to the work done since previous attempt to commit this
> to
> > > > pg17, I can highlight following.
> > > > 1) We're now using more of existing infrastructure including
> > > > adjust_relid_set() and ChangeVarNodes().  The most of complexity is
> > > > still there though.
> > > > 2) We've checked few ways to further simplify this patch.  But yet
> the
> > > > current way still feels to be best possible.
> > > > 3) For sure, several bugs were fixed.
> > > >
> > > > I think we could give it another chance for pg18 after some further
> > > > polishing (at least commit message still needs to be revised).  Any
> > > > thoughts on this?  Tom?
> > > >
> > > I didn't find any mistakes, I just have a refactoring remark. I think
> > > the part where we add non-redundant expressions with the
> > > binfo_candidates, jinfo_candidates
> > > check can be moved to a separate function, otherwise the code is very
> > > repetitive in this place. I did it and attached diff file
> >
> > Thank you.  I've integrated that into a patch.  However, I've to
> > change keep_rinfo_list to be passed by pointer to
> > add_non_redundant_clauses(), because it might be changed in
> > distribute_restrictinfo_to_rels().  Without that there is a case of
> > duplicated clause in regression tests.
> >
> > I've changed 'inner' and 'outer' vise versa in
> > remove_self_joins_one_group() for better readability (I believe that
> > was discussed upthread but lost).  Also, I did a round of improvement
> > for comments and commit message.
>
> I've corrected some spelling error reported by Alexander Lakhin
> privately to me.  Also, I've revised comments around ChangeVarNodes()
> and ChangeVarNodesExtended().  I'm going to continue nitpicking this
> patch during next couple days then push it if no objections.
>
I think a small optimization is possible here.

The whole block that append *rinfo* to exprs is controlled by extra_clause
not NULL.
So It's worth moving the test to the beginning of the block and avoiding it
altogether if that's the case.

trivial patch attached.

best regards,
Ranier Vilela