Re: Removing unneeded self joins

Hi.

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

> On Thu, Feb 13, 2025 at 1:00 AM Alexander Korotkov <aekorotkov(at)gmail(dot)com>
> wrote:
> >
> > On Tue, Feb 11, 2025 at 5:31 PM Alena Rybakina
> > <a(dot)rybakina(at)postgrespro(dot)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(at)gmail(dot)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