Re: Removing unneeded self joins

On 12/10/2023 18:32, Alexander Korotkov wrote:
> On Thu, Oct 5, 2023 at 12:17 PM Andrei Lepikhov
>> We have almost the results we wanted to have. But in the last explain
>> you can see that nothing happened with the OR clause. We should use the
>> expression mutator instead of walker to handle such clauses. But It
>> doesn't process the RestrictInfo node ... I'm inclined to put a solution
>> of this issue off for a while.
> 
> OK.  I think it doesn't worth to eliminate IS NULL quals with this
> complexity (at least at this stage of work).

Yeah. I think It would be meaningful in the case of replacing also 
nested x IS NOT NULL with nothing. But it requires using a mutator 
instead of the walker and may be done more accurately next time.

> I made improvements over the code.  Mostly new comments, grammar
> corrections of existing comments and small refactoring.

Great!

> Also, I found that the  suggestion from David Rowley [1] to qsort
> array of relations to faster find duplicates is still unaddressed.
> I've implemented it.  That helps to evade quadratic complexity with
> large number of relations.

I see. The thread is too long so far, thanks for the catch.

> Also I've incorporated improvements from Alena Rybakina except one for
> skipping SJ removal when no SJ quals is found.  It's not yet clear for
> me if this check fix some cases. But at least optimization got skipped
> in some useful cases (as you can see in regression tests).

Agree. I wouldn't say I like it too. But also, I suggest skipping some 
unnecessary assertions proposed in that patch:
Assert(toKeep->relid != -1); - quite strange. Why -1? Why not all the 
negative numbers, at least?
Assert(is_opclause(orinfo->clause)); - above we skip clauses with 
rinfo->mergeopfamilies == NIL. Each mergejoinable clause is already 
checked as is_opclause.
All these changes (see in the attachment) are optional.

-- 
regards,
Andrey Lepikhov
Postgres Professional