Message-ID: <69a74357-102c-4591-9619-f672076135db@postgrespro.ru>
Date: Fri, 13 Oct 2023 12:42:35 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Removing unneeded self joins
Content-Language: en-US
To: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
Cc: "Gregory Stark (as CFM)" <stark.cfm@gmail.com>,
 =?UTF-8?Q?Micha=C5=82_K=C5=82eczek?= <michal@kleczek.org>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Alexander Korotkov <aekorotkov@gmail.com>
References: <64486b0b-0404-e39e-322d-0801154901f3@postgrespro.ru>
 <d9493d5a-b950-465e-f09b-62c92a23fba1@postgrespro.ru>
 <20221206184643.zya44h5auv6f45zp@awork3.anarazel.de>
 <36579124-f9d9-a51e-38b3-af3511921c7e@postgrespro.ru>
 <88079CBD-B5BB-4C61-A68B-A8C67599E1C7@kleczek.org>
 <CAM-w4HMZXW+hKc8C+WLQ+gdpEkoh9378a2UGKyZRz7mZVerG+Q@mail.gmail.com>
 <c97574c4-13e6-9107-26ab-af2bd626fc85@postgrespro.ru>
 <bcf2f3c5-fbdc-f85b-af3e-ebec616160f6@postgrespro.ru>
 <05469e80-454b-41d7-d04f-421f42bbd4a9@postgrespro.ru>
 <CAPpHfdt9G3EU+vXuqexhweEQoMF-acPUUFuB6hV2Kg0gCy7AjA@mail.gmail.com>
 <ebe66358-428c-439f-85dd-da72c9b2b0c2@postgrespro.ru>
 <CAPpHfdt-0kVV7O==aJEbjY2iGYBu+XBzTHEbPv_6sVNeC7fffQ@mail.gmail.com>
 <48c11b23-9c23-440e-9a3c-020e05196449@postgrespro.ru>
 <CAPpHfdsQkY0Gu7bQ9Ekdyzo_5Xxz74TYKbwmk1on1WAASpxXcQ@mail.gmail.com>
 <9eb86ca9-da3c-417f-8ce1-29c0ef5a4f50@postgrespro.ru>
 <29ddec15-a83f-4f24-b069-6e93c189b169@postgrespro.ru>
 <f052df9a-c714-4c60-92ba-9b5ae7e258b0@postgrespro.ru>
From: "a.rybakina" <a.rybakina@postgrespro.ru>
In-Reply-To: <f052df9a-c714-4c60-92ba-9b5ae7e258b0@postgrespro.ru>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/69a74357-102c-4591-9619-f672076135db%40postgrespro.ru>
Precedence: bulk

On 13.10.2023 12:03, Andrei Lepikhov wrote:
> On 13/10/2023 15:56, a.rybakina wrote:
>>
>>>> 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.
>>>
>> I don't mind about asserts, maybe I misunderstood something in the 
>> patch.
>>
>> About skipping SJ removal when no SJ quals is found, I assume it is 
>> about it:
>>
>> split_selfjoin_quals(root, restrictlist, &selfjoinquals,
>>                                    &otherjoinquals, inner->relid, 
>> outer->relid);
>>
>> +            if (list_length(selfjoinquals) == 0)
>> +             {
>> +                 /*
>> +                  * XXX:
>> +                  * we would detect self-join without quals like 
>> 'x==x' if we had
>> +                  * an foreign key constraint on some of other quals 
>> and this join
>> +                  * haven't any columns from the outer in the target 
>> list.
>> +                  * But it is still complex task.
>> +                  */
>> +                 continue;
>> +             }
>>
>> as far as I remember, this is the place where it is checked that the 
>> SJ list is empty and it is logical, in my opinion, that no 
>> transformations should be performed if no elements are found for them.
> You forget we have "Degenerate" case, as Alexander mentioned above. 
> What if you have something like that:
> SELECT ... FROM A a1, A a2 WHERE a1.id=1 AND a2.id=1;
> In this case, uniqueness can be achieved by the baserestrictinfo 
> "A.id=1", if we have an unique index on this column.
>
Yes, sorry, I missed it. thanks again for the explanation 🙂