Message-ID: <75de38d9-005d-4995-9ecf-a155844dcbcd@postgrespro.ru>
Date: Fri, 3 May 2024 10:19:58 +0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Removing unneeded self joins
To: Tom Lane <tgl@sss.pgh.pa.us>, Alexander Korotkov <aekorotkov@gmail.com>
Cc: Richard Guo <guofenglinux@gmail.com>,
 Alexander Lakhin <exclusion@gmail.com>,
 "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>
References: <64486b0b-0404-e39e-322d-0801154901f3@postgrespro.ru>
 <8800ccf2-ae82-461c-92a1-d4c0a22f8cf2@postgrespro.ru>
 <CAPpHfdvJ3jBQaJQKrf69K++kXi+Tvzn6TXO-DKXj0KVJjvMebg@mail.gmail.com>
 <1bd80083-a13e-4e9f-a7ce-73c3faab521c@postgrespro.ru>
 <CAPpHfdsz8E085YF2VoJe2+h4YAShsh7p--t3kQfQTntiCy63Xg@mail.gmail.com>
 <922623af-872e-4cdc-9238-129b18bee42d@postgrespro.ru>
 <CAPpHfduoAHTp6O7Q8PDe_w=30v0n_06pGe5M65iR4DYcfaYnRw@mail.gmail.com>
 <0ed2cf02-e655-6ec7-a4fe-52fd8d572f65@gmail.com>
 <CAPpHfdsHWbdPVzUdmoKv3dpo8RHJOa=LQR-ZAT+89uvktMs9XA@mail.gmail.com>
 <735b5cd4-568d-fbf7-ad8c-dcfb4c3af86e@gmail.com>
 <CAPpHfdssga9CBQQ6s32t1Xb2Js2WuPfiporbAse334CJiyBb1Q@mail.gmail.com> <81!
 00af4a-d0aa-450d-a09f-541f26b04ac2@postgrespro.ru>
 <CAPpHfdvGVcud2i00pEVH2_GS09c8UsDh6P-SEOLd6YMGOxiNEw@mail.gmail.com>
 <CAMbWs49Q8g1LPVCeNHYv-Y-gFo826ertrg6nYNC9LL=8=zHP3g@mail.gmail.com>
 <CAPpHfdsUiwbGLbBwtdL+UCT5GCUVEU+SqQaaZjtcje3znU2JgQ@mail.gmail.com>
 <2422119.1714691974@sss.pgh.pa.us>
Content-Language: en-AU
From: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
Organization: Postgres Professional
In-Reply-To: <2422119.1714691974@sss.pgh.pa.us>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/75de38d9-005d-4995-9ecf-a155844dcbcd%40postgrespro.ru>
Precedence: bulk

On 5/3/24 06:19, Tom Lane wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
>> I would appreciate your review of this patchset, and review from Andrei as well.
> 
> I hate to say this ... but if we're still finding bugs this
> basic in SJE, isn't it time to give up on it for v17?
> 
> I might feel better about it if there were any reason to think
> these were the last major bugs.  But you have already committed
> around twenty separate fixes for the original SJE patch, and
> now here you come with several more; so it doesn't seem like
> the defect rate has slowed materially.  There can be no doubt
> whatever that the original patch was far from commit-ready.
> 
> I think we should revert SJE for v17 and do a thorough design
> review before trying again in v18.
I need to say I don't see any evidence of bad design.
I think this feature follows the example of 2489d76 [1], 1349d27 [2], 
and partitionwise join features — we get some issues from time to time, 
but these strengths and frequencies are significantly reduced.
First and foremost, this feature is highly isolated: like the PWJ 
feature, you can just disable (not enable?) SJE and it guarantees you 
will avoid the problems.
Secondly, this feature reflects the design decisions the optimiser has 
made before. It raises some questions: Do we really control the 
consistency of our paths and the plan tree? Maybe we hide our 
misunderstanding of its logic by extensively copying expression trees, 
sometimes without fundamental necessity. Perhaps the optimiser needs 
some abstraction layers or reconstruction to reduce the quickly growing 
complexity.
A good example here is [1]. IMO, the new promising feature it has 
introduced isn't worth the complexity it added to the planner.
SJE, much like OR <-> ANY transformation, introduces a fresh perspective 
into the planner: if we encounter a complex, redundant query, it may be 
more beneficial to invest in simplifying the internal query 
representation rather than adding new optimisations that will grapple 
with this complexity.
Also, SJE raised questions I've never seen before, like: Could we 
control the consistency of the PlannerInfo by changing something in the 
logic?
Considering the current state, I don't see any concrete outcomes or 
evidence that a redesign of the feature will lead us to a new path. 
However, I believe the presence of SJE in the core could potentially 
trigger improvements in the planner. As a result, I vote to stay with 
the feature. But remember, as an author, I'm not entirely objective, so 
let's wait for alternative opinions.

[1] Make Vars be outer-join-aware
[2] Improve performance of ORDER BY / DISTINCT aggregates

-- 
regards,
Andrei Lepikhov
Postgres Professional