Message-ID: <98f604ad-c5d2-4b2d-86a7-0ef031ff1a78@postgrespro.ru>
Date: Wed, 1 Nov 2023 10:55:45 +0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: BUG #18170: Unexpected error: no relation entry for relid 3
Content-Language: en-US
To: Alexander Korotkov <aekorotkov@gmail.com>,
 Richard Guo <guofenglinux@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Vik Fearing <vik@postgresfriends.org>,
 zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org,
 Alexander Korotkov <akorotkov@postgresql.org>
References: <18170-f1d17bf9a0d58b24@postgresql.org>
 <1189851.1698340331@sss.pgh.pa.us>
 <011bcae5-7840-42c7-a50c-b406189e2e78@postgrespro.ru>
 <CAPpHfdsiN0norSHfw3Ujemf=RuPzMxt5PRDiyv0vqUdwSBDtoA@mail.gmail.com>
 <721542ce-79df-469e-93e9-811fadb5bd0c@postgrespro.ru>
 <2eb87c2e-7e6f-4002-8df3-8fac3aa6a037@postgrespro.ru>
 <CAMbWs494m3AjhHry_HFCFi_SJ+6-PaeMRbx7jK39ZHmK0MVOUA@mail.gmail.com>
 <c0a5b39d-8a80-47fb-bd34-6d769f88df8f@postgrespro.ru>
 <CAPpHfduPMdLm42uwuXqM1weUKSYp=2Kf-_XU2TjZsizeZ3qoWQ@mail.gmail.com>
 <CAPpHfdv_6K_sv5sL_=9EiP7Lve81WLSdiLrc22afpmg_whyxtg@mail.gmail.com>
 <2141583.1698595016@sss.pgh.pa.us>
 <CAMbWs489RP3CFKwtWoG2m=qD5byfS_9_13yVU5wyknDTb3GToQ@mail.gmail.com>
 <fc183714-a61c-4de7-97e0-b1c376923aef@postgrespro.ru>
 <CAMbWs48FTQtO=ARgjbHqz5fYtOhVUAbdEC1PpnMsjsaRsOENeQ@mail.gmail.com>
 <7332bc61-a9fa-464b-b5a4-4e8e0f764a64@postgrespro.ru>
 <CAMbWs4_4Udor4YfMi6YiM+r1uj-UGbbf7S4yXrkXo4vXLiiYNw@mail.gmail.com>
 <CAPpHfdseB13zJJPZuBORevRnZ0vcFyUaaJeSGfAysX7S5er+EQ@mail.gmail.com>
From: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
Organization: Postgres Professional
In-Reply-To: 
 <CAPpHfdseB13zJJPZuBORevRnZ0vcFyUaaJeSGfAysX7S5er+EQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/98f604ad-c5d2-4b2d-86a7-0ef031ff1a78%40postgrespro.ru>
Precedence: bulk

On 31/10/2023 15:46, Alexander Korotkov wrote:
> On Tue, Oct 31, 2023 at 9:37 AM Richard Guo <guofenglinux@gmail.com> wrote:
>> On Tue, Oct 31, 2023 at 1:05 PM Andrei Lepikhov <a.lepikhov@postgrespro.ru> wrote:
>>>
>>> On 30/10/2023 13:24, Richard Guo wrote:
>>>> Yeah, that's what I meant.  We need to ensure that root->parse
>>>> references the same Query structure during the whole subquery_planner()
>>>> for this patch to work, which seems hacky, and error-prone for future
>>>> development.  If we really want to do so, at least we need to emphasize
>>>> this point in the comment of subquery_planner().
>>>
>>> Okay, let's go another way and try to imagine what additional
>>> opportunities it will give us in the future? I mean, save unchanged a
>>> rte->subquery tree and, simultaneously, have some transformed version in
>>> the subroot->parse field.
>>> I can imagine kind of a subquery replanning procedure in the case we
>>> realized during higher-level query planning that the underlying subquery
>>> is not optimal.
>>> Such a picture makes sense, and I can agree with your words. From this
>>> point of view, all links to the subquery must reference the subroot
>>> structures, and the patch you have proposed is the best solution. And
>>> the SJE feature works correctly.
>>> If we don't imagine the picture I have drawn above, it is reasonable to
>>> save memory and not alter the parse tree while removing unneeded joins.
>>> I think, in that case, we can use the walker procedure instead of a mutator.
>>
>>
>> Well, I think what happens here is that the new SJE code alters the
>> subquery (by removing ref_2) and that change is not propagated into the
>> parent level.  So in the parent level, when we look at the subquery's
>> original targetlist (which remains unchanged) against 'subroot' (which
>> has been changed accordingly), we'd have problem.
>>
>> AFAICS there are two solutions being discussed:
>>
>> 1) We propagate the change to the subquery into the parent level by
>> ensuring that root->parse references the same Query structure during the
>> whole subquery_planner().
> 
> Even if we don't pick this option to resolve the current problem, I
> would say I'm not particularly happy that root->parse and
> corresponding parent_root->simple_rte_array[]->subquery are distinct
> copies without a strong reason for that.
I don't happy with this approach too.
> 
>> 2) In the parent level, we look at the changed subquery instead of the
>> original rte->subquery.  IOW, we look at subroot->parse->targetList
>> instead of subquery->targetList.
>>
>> Personally I prefer the second solution because it seems more natural to
>> look at 'subroot->parse' together with 'subroot' in estimate_num_groups
>> and it does not introduce new constraint to subquery_planner().
> 
> 3) We may decide to introduce "alias" relids.  Instead of deleting the
> relid completely, we may decide to make it an alias of another relid.
> See the attached patch to illustrate this approach.  I guess it could
> allow us to get rid of a significant part of relid replacement login
> in d3d55ce571.  I'm not sure about potential problems with this
> approach though.

By and large, the idea of an alias looks productive. It is really 
cheaper and simpler to make an alias. But we still need to replace 
relids in clauses. This approach requires a lot of additional work, as I 
see. But right now, we should agree on some more straightforward solution.

In Richard's approach No.2, I see some rational grain: living with such 
a quick fix for some time, we can find additional misleading references 
to the 'rte->subquery' in the upper query.

Can we postpone the subquery trees sync part of the patch and commit 
Richard's fix?

-- 
regards,
Andrei Lepikhov
Postgres Professional