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.
> 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.
------
Regards,
Alexander Korotkov
