Message-ID: <85d91f13-b6d3-4d5c-8866-f2d9533c042a@postgrespro.ru>
Date: Thu, 30 Nov 2023 14:21:50 +0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: BUG #18187: Unexpected error: "variable not found in subplan
 target lists" triggered by JOIN
Content-Language: en-US
To: Richard Guo <guofenglinux@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alexander Korotkov <aekorotkov@gmail.com>, zuming.jiang@inf.ethz.ch,
 pgsql-bugs@lists.postgresql.org,
 PG Bug reporting form <noreply@postgresql.org>
References: <18187-831da249cbd2ff8e@postgresql.org>
 <491881ef-f1c7-4a86-b5dd-dcad55a88ab4@postgrespro.ru>
 <CAMbWs4_yrqMYXd+zXc4Y6KjnyA8xPZE6aiAW6k2Y10WaNhwRhw@mail.gmail.com>
 <CAMbWs4-ns73VF9gi37q61G3dS6Xuos+HtryMaBh37WQn=BsaJw@mail.gmail.com>
 <CAMbWs4_WeARU0oYH1sMn03VYzw-qZQ_xwUNq7r6TbDTH+VgXng@mail.gmail.com>
 <3883031.1701106936@sss.pgh.pa.us>
 <CAMbWs48UBgxyzNKgu7ZrWDGy-6y33+uC8znyDceRzaDb=+XOcw@mail.gmail.com>
From: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
Organization: Postgres Professional
In-Reply-To: 
 <CAMbWs48UBgxyzNKgu7ZrWDGy-6y33+uC8znyDceRzaDb=+XOcw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/85d91f13-b6d3-4d5c-8866-f2d9533c042a%40postgrespro.ru>
Precedence: bulk

On 28/11/2023 14:03, Richard Guo wrote:
> 
> On Tue, Nov 28, 2023 at 1:42 AM Tom Lane <tgl@sss.pgh.pa.us 
> <mailto:tgl@sss.pgh.pa.us>> wrote:
> 
>     Richard Guo <guofenglinux@gmail.com <mailto:guofenglinux@gmail.com>>
>     writes:
>      > After some more thought, I think PHVs should not impose any
>     constraints
>      > on removing self joins.  If the removed rel is contained in the
>     relids
>      > that a PHV is evaluated/needed at or laterally references, it can
>     just
>      > be replaced with the rel that is kept.
> 
>     I experimented with trying to break this patch using this test case:
> 
>     create table t (id int primary key, f1 text, f2 text);
>     create table i (f0 int);
> 
>     explain verbose
>     select * from i left join (
>        (select *, 1 as x from t tss1) t1 join
>        (select *, 2 as y from t tss2) t2
>        on t1.id <http://t1.id> = t2.id <http://t2.id>
>     ) on i.f0 = t1.id <http://t1.id>;
> 
> 
> Thanks for the query!
> 
>     It seems to successfully remove the self-join between tss1 and tss2,
>     but there is something wrong.  If you examine the PlannerInfo just
>     after remove_useless_self_joins, you will notice that the two
>     PlaceHolderVars (for x and y) in the placeholder_list both have
> 
>               :phrels (b 7)
>               :phnullingrels (b)
> 
>     where beforehand x had
> 
>               :phrels (b 6)
>               :phnullingrels (b 5)
> 
>     and y had
> 
>               :phrels (b 7)
>               :phnullingrels (b 5)
> 
>     It's correct to move both phrels locations to rel 7 (the surviving
>     self-joined rel) but what became of the reference to nullingrel 5?
>     That seems clearly wrong, since we have not removed the left join.
> 
> 
> What you noticed before remove_useless_self_joins seems not right to me.
> The PHVs in placeholder_list are supposed to have empty phnullingrels by
> convention, as explained in find_placeholder_info.
> 
> * By convention, phinfo->ph_var->phnullingrels is always empty, since the
> * PlaceHolderInfo represents the initially-calculated state of the
> * PlaceHolderVar.
> 
>     Much worse, if you look around elsewhere in the data structure,
>     particularly at the processed_tlist, you find instances of the
>     PlaceHolderVars that have not been updated and still have the
>     old values such as ":phrels (b 6)".
> 
> 
> Good catch!  We fail to handle PlaceHolderVar case in sje_walker and
> replace_varno_walker, which is wrong.  Fixed in v3 patch.
> 
>     It's not apparent to me why the cross-checks we have in setrefs.c
>     and elsewhere don't detect a problem with this.  But it seems
>     clear that remove_useless_self_joins is still several bricks
>     shy of a load in terms of fully updating the data structure for a
>     join removal.  Probably with some more work to add complication
>     to this test case, we could demonstrate an observable failure.
> 
> 
> Fair point.  I have the same concern that we still have other loose ends
> in SJE updating necessary data structures in self join removal.  And I
> noticed that Andres expressed the same concern in [1].  I think we
> should come up with some solution to detect/avoid such problems, but I
> imagine that that should be a seperate patch.

Agree. PlannerInfo is a hot structure where many new features try to 
save some data. The history of this patch has shown, for example, 
JoinDomains. Finding field jd_relids, which could contain the link to 
the removing relation, was tricky.
I still have no idea how to check it internally except by attaching a 
comment to the PlannerInfo structure.

-- 
regards,
Andrei Lepikhov
Postgres Professional