pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
24+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2023-12-24 0a93f803f4 Fix a comment for remove_self_joins_recurse()
2023-12-24 b5fb6736ed Don't constrain self-join removal due to PHVs
2023-12-24 8a8ed916f7 Handle PlaceHolderVar case in replace_varno_walker
2023-11-10 b7f315c9d7 Fix how SJE checks against PHVs
* BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-07 17:04 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 24+ messages in thread
From: PG Bug reporting form @ 2023-11-07 17:04 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: zuming.jiang@inf.ethz.ch
The following bug has been logged on the website:
Bug reference: 18187
Logged by: Zuming Jiang
Email address: zuming.jiang@inf.ethz.ch
PostgreSQL version: 16.0
Operating system: Ubuntu 20.04
Description:
My fuzzer finds a bug in Postgres 17devel, which triggers an unexpected
error.
--- Set up database ---
create table t2 (vkey int4, c9 text, primary key(vkey));
create view t4 as select 1 as c_2, 1 as c_3;
create view t5 as
select
1 as c_0,
case when '1' ~<=~ ref_0.c9 then 1 else 1 end as c_3
from
((t2 as ref_0
inner join t2 as ref_1
on (ref_0.vkey = ref_1.vkey))
right outer join t2 as ref_2
on (ref_1.vkey = ref_2.vkey ));
------------------------
The fuzzer generates a test case:
--- Test case ---
select
ref_5.c_3
from
(((select ref_1.c_3 as c_0 from t4 as ref_1) as subq_0
right outer join t5 as ref_5
on (subq_0.c_0 = ref_5.c_0))
right outer join t4 as ref_6
on (subq_0.c_0 = ref_6.c_2));
------------------------
--- Expected behavior ---
The test case should not trigger any error.
--- Actual behavior ---
The test case trigger an error:
ERROR: variable not found in subplan target lists
--- Postgres version ---
Github commit: 3c551ebede46194237f82062b54b92e474b5c743
Version: PostgreSQL 17devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
9.4.0-1ubuntu1~20.04.1) 9.4.0, 64-bit
--- Platform information ---
Platform: Ubuntu 20.04
Kernel: Linux 5.4.0-147-generic
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-08 15:42 Andrei Lepikhov <a.lepikhov@postgrespro.ru>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 2 replies; 24+ messages in thread
From: Andrei Lepikhov @ 2023-11-08 15:42 UTC (permalink / raw)
To: zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On 8/11/2023 00:04, PG Bug reporting form wrote:
> The following bug has been logged on the website:
>
> Bug reference: 18187
> Logged by: Zuming Jiang
> Email address: zuming.jiang@inf.ethz.ch
> PostgreSQL version: 16.0
> Operating system: Ubuntu 20.04
> Description:
>
> My fuzzer finds a bug in Postgres 17devel, which triggers an unexpected
> error.
>
> --- Set up database ---
> create table t2 (vkey int4, c9 text, primary key(vkey));
> create view t4 as select 1 as c_2, 1 as c_3;
> create view t5 as
> select
> 1 as c_0,
> case when '1' ~<=~ ref_0.c9 then 1 else 1 end as c_3
> from
> ((t2 as ref_0
> inner join t2 as ref_1
> on (ref_0.vkey = ref_1.vkey))
> right outer join t2 as ref_2
> on (ref_1.vkey = ref_2.vkey ));
> ------------------------
>
> The fuzzer generates a test case:
>
> --- Test case ---
> select
> ref_5.c_3
> from
> (((select ref_1.c_3 as c_0 from t4 as ref_1) as subq_0
> right outer join t5 as ref_5
> on (subq_0.c_0 = ref_5.c_0))
> right outer join t4 as ref_6
> on (subq_0.c_0 = ref_6.c_2));
> ------------------------
>
> --- Expected behavior ---
> The test case should not trigger any error.
>
> --- Actual behavior ---
> The test case trigger an error:
>
> ERROR: variable not found in subplan target lists
>
> --- Postgres version ---
> Github commit: 3c551ebede46194237f82062b54b92e474b5c743
> Version: PostgreSQL 17devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu
> 9.4.0-1ubuntu1~20.04.1) 9.4.0, 64-bit
>
> --- Platform information ---
> Platform: Ubuntu 20.04
> Kernel: Linux 5.4.0-147-generic
>
Great,
I would like to have such fuzzer at the stage of development ;)
The issue looks like previous one, related to new SJE feature.
Reduced case:
CREATE TABLE t2 (vkey int4, c9 text, primary key(vkey));
SELECT * FROM (
SELECT CASE WHEN '1' = ref_0.c9 THEN 1 ELSE 1 END AS c_3
FROM t2 as ref_0
JOIN t2 AS ref_1
ON ref_0.vkey = ref_1.vkey
RIGHT OUTER JOIN t2 AS ref_2
ON ref_1.vkey = ref_2.vkey) AS t5
RIGHT OUTER JOIN (SELECT 1 AS c_2) AS t4
ON t4.c_2 IS NOT NULL;
The key problem lies in the 'CASE' statement.
--
regards,
Andrei Lepikhov
Postgres Professional
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-08 17:18 Zu-Ming Jiang <zuming.jiang@inf.ethz.ch>
parent: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
1 sibling, 0 replies; 24+ messages in thread
From: Zu-Ming Jiang @ 2023-11-08 17:18 UTC (permalink / raw)
To: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; pgsql-bugs@lists.postgresql.org; PG Bug reporting form <noreply@postgresql.org>
> Great,
> I would like to have such fuzzer at the stage of development 😉
Thanks for your appreciation!
I would like to open source the tool after my research paper on this
tool get accepted.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-09 03:54 Richard Guo <guofenglinux@gmail.com>
parent: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
1 sibling, 2 replies; 24+ messages in thread
From: Richard Guo @ 2023-11-09 03:54 UTC (permalink / raw)
To: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On Wed, Nov 8, 2023 at 11:42 PM Andrei Lepikhov <a.lepikhov@postgrespro.ru>
wrote:
> The issue looks like previous one, related to new SJE feature.
>
> Reduced case:
> CREATE TABLE t2 (vkey int4, c9 text, primary key(vkey));
> SELECT * FROM (
> SELECT CASE WHEN '1' = ref_0.c9 THEN 1 ELSE 1 END AS c_3
> FROM t2 as ref_0
> JOIN t2 AS ref_1
> ON ref_0.vkey = ref_1.vkey
> RIGHT OUTER JOIN t2 AS ref_2
> ON ref_1.vkey = ref_2.vkey) AS t5
> RIGHT OUTER JOIN (SELECT 1 AS c_2) AS t4
> ON t4.c_2 IS NOT NULL;
>
> The key problem lies in the 'CASE' statement.
I've looked into this a little bit. I think it's caused by the SJE
logic not properly removing references from PHVs. Specifically, it
fails to replace the ref_0's Vars within phv->phexpr, leading them to be
added in ref_2/ref_1 join's targetlist.
Also, I noticed that in remove_rel_from_query() we perform replace_relid
for phv->phrels twice at line 475 and 478, which seems not right to me.
475 phv->phrels = replace_relid(phv->phrels, relid, subst);
476 phv->phrels = replace_relid(phv->phrels, ojrelid, subst);
477 phinfo->ph_lateral = replace_relid(phinfo->ph_lateral, relid, subst);
478 phinfo->ph_var->phrels = replace_relid(phinfo->ph_var->phrels,
relid, subst);
Attached is a hotfix.
Thanks
Richard
Attachments:
v1-0001-Fix-how-SJE-removes-references-from-PHVs.patch (application/octet-stream, 3.2K) download
From 75a2c7a00d1c02e996f523b3f6350f7d5d6b4f91 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Thu, 9 Nov 2023 11:28:26 +0800
Subject: [PATCH v1] Fix how SJE removes references from PHVs
---
src/backend/optimizer/plan/analyzejoins.c | 4 ++--
src/test/regress/expected/join.out | 19 +++++++++++++++++++
src/test/regress/sql/join.sql | 8 ++++++++
3 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 953e381d6f..271f694d99 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -474,9 +474,9 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
/* ph_needed might or might not become empty */
phv->phrels = replace_relid(phv->phrels, relid, subst);
phv->phrels = replace_relid(phv->phrels, ojrelid, subst);
- phinfo->ph_lateral = replace_relid(phinfo->ph_lateral, relid, subst);
- phinfo->ph_var->phrels = replace_relid(phinfo->ph_var->phrels, relid, subst);
Assert(!bms_is_empty(phv->phrels));
+ replace_varno((Node *) phv->phexpr, relid, subst);
+ phinfo->ph_lateral = replace_relid(phinfo->ph_lateral, relid, subst);
Assert(phv->phnullingrels == NULL); /* no need to adjust */
}
}
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 892ea5f170..ddc4e69232 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -6776,6 +6776,25 @@ SELECT c3.code FROM emp1 c3;
-> Seq Scan on emp1 c3
(7 rows)
+-- Check that SJE removes references from PHVs correctly
+explain (costs off)
+select * from emp1 t1 left join
+ (select coalesce(t3.code, 1) from emp1 t2
+ left join (emp1 t3 join emp1 t4 on t3.id = t4.id)
+ on true)
+on true;
+ QUERY PLAN
+----------------------------------------------------
+ Nested Loop Left Join
+ -> Seq Scan on emp1 t1
+ -> Materialize
+ -> Nested Loop Left Join
+ -> Seq Scan on emp1 t2
+ -> Materialize
+ -> Seq Scan on emp1 t4
+ Filter: (id IS NOT NULL)
+(8 rows)
+
-- We can remove the join even if we find the join can't duplicate rows and
-- the base quals of each side are different. In the following case we end up
-- moving quals over to s1 to make it so it can't match any rows.
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 559c36dc07..a41787d1f1 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2580,6 +2580,14 @@ WHERE c2.id IS NOT NULL
EXCEPT ALL
SELECT c3.code FROM emp1 c3;
+-- Check that SJE removes references from PHVs correctly
+explain (costs off)
+select * from emp1 t1 left join
+ (select coalesce(t3.code, 1) from emp1 t2
+ left join (emp1 t3 join emp1 t4 on t3.id = t4.id)
+ on true)
+on true;
+
-- We can remove the join even if we find the join can't duplicate rows and
-- the base quals of each side are different. In the following case we end up
-- moving quals over to s1 to make it so it can't match any rows.
--
2.31.0
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-09 04:51 Andrei Lepikhov <a.lepikhov@postgrespro.ru>
parent: Richard Guo <guofenglinux@gmail.com>
1 sibling, 1 reply; 24+ messages in thread
From: Andrei Lepikhov @ 2023-11-09 04:51 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On 9/11/2023 10:54, Richard Guo wrote:
>
> On Wed, Nov 8, 2023 at 11:42 PM Andrei Lepikhov
> <a.lepikhov@postgrespro.ru <mailto:a.lepikhov@postgrespro.ru>> wrote:
>
> The issue looks like previous one, related to new SJE feature.
>
> Reduced case:
> CREATE TABLE t2 (vkey int4, c9 text, primary key(vkey));
> SELECT * FROM (
> Â Â SELECT CASE WHEN '1' = ref_0.c9 THEN 1 ELSE 1 END AS c_3
> FROM t2 as ref_0
> Â Â JOIN t2 AS ref_1
> Â Â ON ref_0.vkey = ref_1.vkey
> Â Â Â RIGHT OUTER JOIN t2 AS ref_2
> Â Â Â ON ref_1.vkey = ref_2.vkey) AS t5
> Â Â RIGHT OUTER JOIN (SELECT 1 AS c_2) AS t4
> Â Â ON t4.c_2 IS NOT NULL;
>
> The key problem lies in the 'CASE' statement.
>
>
> I've looked into this a little bit. I think it's caused by the SJE
> logic not properly removing references from PHVs. Specifically, it
> fails to replace the ref_0's Vars within phv->phexpr, leading them to be
> added in ref_2/ref_1 join's targetlist.
>
> Also, I noticed that in remove_rel_from_query() we perform replace_relid
> for phv->phrels twice at line 475 and 478, which seems not right to me.
>
> Â 475 Â phv->phrels = replace_relid(phv->phrels, relid, subst);
> Â 476 Â phv->phrels = replace_relid(phv->phrels, ojrelid, subst);
> Â 477 Â phinfo->ph_lateral = replace_relid(phinfo->ph_lateral, relid,
> subst);
> Â 478 Â phinfo->ph_var->phrels = replace_relid(phinfo->ph_var->phrels,
> relid, subst);
Thanks a lot!
Your patch looks correct. We really have been missing replacing the
PlaceHolderVar expression references for all the development time.
The test in join.sql works correctly. Should we add a reference to the
bug that triggered the issue as a comment to the test? Also, to be sure,
maybe add column t4.code into the list of the coalesce parameters?
--
regards,
Andrei Lepikhov
Postgres Professional
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-09 11:08 Richard Guo <guofenglinux@gmail.com>
parent: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 1 reply; 24+ messages in thread
From: Richard Guo @ 2023-11-09 11:08 UTC (permalink / raw)
To: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On Thu, Nov 9, 2023 at 12:51 PM Andrei Lepikhov <a.lepikhov@postgrespro.ru>
wrote:
> ... Should we add a reference to the
> bug that triggered the issue as a comment to the test?
Yeah, we can do that. I guess something like:
-- Check that SJE removes references from PHVs correctly (bug #18187)
> Also, to be sure,
> maybe add column t4.code into the list of the coalesce parameters?
I don't think it's necessary. It is t3 that SJE would remove, so it's
sufficient to have t3's Vars in the PHV expression to trigger this
error.
Thanks
Richard
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-09 11:21 Andrei Lepikhov <a.lepikhov@postgrespro.ru>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Andrei Lepikhov @ 2023-11-09 11:21 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; Alexander Korotkov <akorotkov@postgresql.org>; +Cc: zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On 9/11/2023 18:08, Richard Guo wrote:
>
> On Thu, Nov 9, 2023 at 12:51 PM Andrei Lepikhov
> <a.lepikhov@postgrespro.ru <mailto:a.lepikhov@postgrespro.ru>> wrote:
>
> ... Should we add a reference to the
> bug that triggered the issue as a comment to the test?
>
>
> Yeah, we can do that. I guess something like:
>
> -- Check that SJE removes references from PHVs correctly (bug #18187)
>
> Also, to be sure,
> maybe add column t4.code into the list of the coalesce parameters?
>
>
> I don't think it's necessary. It is t3 that SJE would remove, so it's
> sufficient to have t3's Vars in the PHV expression to trigger this
> error.
Agree. I've considered the situations when something in the internal
planner logic changes, and relations could arrive in a different order.
In that case, we sort relids, and have determined behaviour. So, do we
wait for Alexander's glance?
--
regards,
Andrei Lepikhov
Postgres Professional
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-09 12:25 Alexander Korotkov <aekorotkov@gmail.com>
parent: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 0 replies; 24+ messages in thread
From: Alexander Korotkov @ 2023-11-09 12:25 UTC (permalink / raw)
To: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: Richard Guo <guofenglinux@gmail.com>; Alexander Korotkov <akorotkov@postgresql.org>; zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On Thu, Nov 9, 2023 at 1:21 PM Andrei Lepikhov
<a.lepikhov@postgrespro.ru> wrote:
> On 9/11/2023 18:08, Richard Guo wrote:
> >
> > On Thu, Nov 9, 2023 at 12:51 PM Andrei Lepikhov
> > <a.lepikhov@postgrespro.ru <mailto:a.lepikhov@postgrespro.ru>> wrote:
> >
> > ... Should we add a reference to the
> > bug that triggered the issue as a comment to the test?
> >
> >
> > Yeah, we can do that. I guess something like:
> >
> > -- Check that SJE removes references from PHVs correctly (bug #18187)
> >
> > Also, to be sure,
> > maybe add column t4.code into the list of the coalesce parameters?
> >
> >
> > I don't think it's necessary. It is t3 that SJE would remove, so it's
> > sufficient to have t3's Vars in the PHV expression to trigger this
> > error.
>
> Agree. I've considered the situations when something in the internal
> planner logic changes, and relations could arrive in a different order.
> In that case, we sort relids, and have determined behaviour. So, do we
> wait for Alexander's glance?
LGMT, pushed!
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-10 08:20 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
1 sibling, 2 replies; 24+ messages in thread
From: Richard Guo @ 2023-11-10 08:20 UTC (permalink / raw)
To: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; Alexander Korotkov <aekorotkov@gmail.com>; +Cc: zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On Thu, Nov 9, 2023 at 11:54 AM Richard Guo <guofenglinux@gmail.com> wrote:
> I've looked into this a little bit. I think it's caused by the SJE
> logic not properly removing references from PHVs.
>
Looking closer at the codes, I think we still have some loose ends
regarding how SJE handles PHVs.
1) When we check PHVs in remove_self_joins_one_group() to see if there
is any PHV that prevents us from removing the self join, we do not check
ph_lateral. This is dangerous because it leaves us with no guarantee of
what the Assert in remove_rel_from_query() asserts:
Assert(!bms_is_member(relid, phinfo->ph_lateral));
To illustrate this problem, look at the query below which would trigger
this Assert.
create table t (a int primary key, b int);
explain (costs off)
select * from t t1 join t t2 on t1.a = t2.a left join
lateral (select t1.a as t1a, * from generate_series(1,1) t3) s on true;
server closed the connection unexpectedly
2) Currently remove_self_joins_one_group() checks PHVs as below
/* there isn't any other place to eval PHV */
if (bms_is_subset(phinfo->ph_eval_at, joinrelids) ||
bms_is_subset(phinfo->ph_needed, joinrelids))
break;
I'm wondering if we can relax this restriction because it seems to me
that a PHV evaluated/needed at or below the self join should not have
problem if we remove the self join. For instance,
explain (costs off)
select * from generate_series(1,10) t1(a) left join lateral
(select t1.a as t1a, t2.a from t t2 join t t3 on t2.a = t3.a) on true;
QUERY PLAN
-------------------------------------------
Nested Loop Left Join
-> Function Scan on generate_series t1
-> Hash Join
Hash Cond: (t2.a = t3.a)
-> Seq Scan on t t2
-> Hash
-> Seq Scan on t t3
(7 rows)
I think the t2/t3 join can actually be removed.
And now I have a vague feeling that PHVs should not impose any
constraints on removing self joins. But I'm not sure.
It would be great if we could have Tom's perspective on this. So I've
added Tom in the cc list.
FWIW, attached is a band-aid fix for the Assert failure issue in 1), in
case we want to fix it first before we discuss this topic further.
Thanks
Richard
Attachments:
v1-0001-Fix-how-SJE-checks-against-PHVs.patch (application/octet-stream, 3.3K) download
From 6438e8845234dbf9e47a3213390a4d7b60844ca3 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Fri, 10 Nov 2023 15:35:11 +0800
Subject: [PATCH v1] Fix how SJE checks against PHVs
Do not remove self joins if the rel to be removed is laterally referenced by
PHVs.
---
src/backend/optimizer/plan/analyzejoins.c | 4 ++--
src/test/regress/expected/join.out | 14 ++++++++++++++
src/test/regress/sql/join.sql | 5 +++++
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 271f694d99..b9be19a687 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -476,7 +476,6 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
phv->phrels = replace_relid(phv->phrels, ojrelid, subst);
Assert(!bms_is_empty(phv->phrels));
replace_varno((Node *) phv->phexpr, relid, subst);
- phinfo->ph_lateral = replace_relid(phinfo->ph_lateral, relid, subst);
Assert(phv->phnullingrels == NULL); /* no need to adjust */
}
}
@@ -2134,7 +2133,8 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
/* there isn't any other place to eval PHV */
if (bms_is_subset(phinfo->ph_eval_at, joinrelids) ||
- bms_is_subset(phinfo->ph_needed, joinrelids))
+ bms_is_subset(phinfo->ph_needed, joinrelids) ||
+ bms_is_member(r, phinfo->ph_lateral))
break;
}
if (lc)
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index b60f5a67c1..3cc8fd6e6d 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -6821,6 +6821,20 @@ on true;
Filter: (id IS NOT NULL)
(8 rows)
+-- Check that SJE does not remove self joins if a PHV references the removed rel laterally
+explain (costs off)
+select * from emp1 t1 join emp1 t2 on t1.id = t2.id left join
+ lateral (select t1.id as t1id, * from generate_series(1,1) t3) s on true;
+ QUERY PLAN
+---------------------------------------------------
+ Nested Loop Left Join
+ -> Nested Loop
+ -> Seq Scan on emp1 t1
+ -> Index Scan using emp1_pkey on emp1 t2
+ Index Cond: (id = t1.id)
+ -> Function Scan on generate_series t3
+(6 rows)
+
-- We can remove the join even if we find the join can't duplicate rows and
-- the base quals of each side are different. In the following case we end up
-- moving quals over to s1 to make it so it can't match any rows.
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index fbaee480d3..670379129c 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2600,6 +2600,11 @@ select * from emp1 t1 left join
on true)
on true;
+-- Check that SJE does not remove self joins if a PHV references the removed rel laterally
+explain (costs off)
+select * from emp1 t1 join emp1 t2 on t1.id = t2.id left join
+ lateral (select t1.id as t1id, * from generate_series(1,1) t3) s on true;
+
-- We can remove the join even if we find the join can't duplicate rows and
-- the base quals of each side are different. In the following case we end up
-- moving quals over to s1 to make it so it can't match any rows.
--
2.31.0
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-10 21:02 Alexander Korotkov <aekorotkov@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
1 sibling, 0 replies; 24+ messages in thread
From: Alexander Korotkov @ 2023-11-10 21:02 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On Fri, Nov 10, 2023 at 10:21 AM Richard Guo <guofenglinux@gmail.com> wrote:
> FWIW, attached is a band-aid fix for the Assert failure issue in 1), in
> case we want to fix it first before we discuss this topic further.
I've pushed this, thank you. I think we should keep the source tree
in working order, while waiting for SJ removal in lateral to be fixed.
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-20 08:49 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
1 sibling, 2 replies; 24+ messages in thread
From: Richard Guo @ 2023-11-20 08:49 UTC (permalink / raw)
To: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; Alexander Korotkov <aekorotkov@gmail.com>; +Cc: zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On Fri, Nov 10, 2023 at 4:20 PM Richard Guo <guofenglinux@gmail.com> wrote:
> Looking closer at the codes, I think we still have some loose ends
> regarding how SJE handles PHVs.
>
> ...
>
> 2) Currently remove_self_joins_one_group() checks PHVs as below
>
> /* there isn't any other place to eval PHV */
> if (bms_is_subset(phinfo->ph_eval_at, joinrelids) ||
> bms_is_subset(phinfo->ph_needed, joinrelids))
> break;
>
> I'm wondering if we can relax this restriction because it seems to me
> that a PHV evaluated/needed at or below the self join should not have
> problem if we remove the self join.
>
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.
Attached is a patch to remove such constraints. Any comments or
feedback are welcome.
Thanks
Richard
Attachments:
v2-0001-Don-t-constrain-self-join-removal-due-to-PHVs.patch (application/octet-stream, 5.5K) download
From 15779d38a9fb7584a58e7b8dc49e1629d1aaee0f Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 20 Nov 2023 16:14:24 +0800
Subject: [PATCH v2] Don't constrain self join removal due to PHVs
---
src/backend/optimizer/plan/analyzejoins.c | 18 +++--------
src/test/regress/expected/join.out | 39 ++++++++++++++++-------
src/test/regress/sql/join.sql | 10 ++++--
3 files changed, 39 insertions(+), 28 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index b9be19a687..783f136b0a 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -453,7 +453,7 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
{
PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(l);
- Assert(!bms_is_member(relid, phinfo->ph_lateral));
+ Assert(sjinfo == NULL || !bms_is_member(relid, phinfo->ph_lateral));
if (bms_is_subset(phinfo->ph_needed, joinrelids) &&
bms_is_member(relid, phinfo->ph_eval_at) &&
!bms_is_member(ojrelid, phinfo->ph_eval_at))
@@ -472,6 +472,8 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
phinfo->ph_needed = replace_relid(phinfo->ph_needed, relid, subst);
phinfo->ph_needed = replace_relid(phinfo->ph_needed, ojrelid, subst);
/* ph_needed might or might not become empty */
+ phinfo->ph_lateral = replace_relid(phinfo->ph_lateral, relid, subst);
+ /* ph_lateral might or might not be empty */
phv->phrels = replace_relid(phv->phrels, relid, subst);
phv->phrels = replace_relid(phv->phrels, ojrelid, subst);
Assert(!bms_is_empty(phv->phrels));
@@ -2125,20 +2127,8 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
joinrelids = bms_add_member(joinrelids, k);
/*
- * Be safe to do not remove tables participated in complicated PH
+ * PHVs should not impose any constraints on removing self joins.
*/
- foreach(lc, root->placeholder_list)
- {
- PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(lc);
-
- /* there isn't any other place to eval PHV */
- if (bms_is_subset(phinfo->ph_eval_at, joinrelids) ||
- bms_is_subset(phinfo->ph_needed, joinrelids) ||
- bms_is_member(r, phinfo->ph_lateral))
- break;
- }
- if (lc)
- continue;
/*
* At this stage, joininfo lists of inner and outer can contain
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 2c73270143..69427287ff 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -6821,20 +6821,37 @@ on true;
Filter: (id IS NOT NULL)
(8 rows)
--- Check that SJE does not remove self joins if a PHV references the removed
--- rel laterally.
-explain (costs off)
+-- Check that PHVs do not impose any constraints on removing self joins
+explain (verbose, costs off)
select * from emp1 t1 join emp1 t2 on t1.id = t2.id left join
lateral (select t1.id as t1id, * from generate_series(1,1) t3) s on true;
- QUERY PLAN
----------------------------------------------------
+ QUERY PLAN
+----------------------------------------------------------
Nested Loop Left Join
- -> Nested Loop
- -> Seq Scan on emp1 t1
- -> Index Scan using emp1_pkey on emp1 t2
- Index Cond: (id = t1.id)
- -> Function Scan on generate_series t3
-(6 rows)
+ Output: t2.id, t2.code, t2.id, t2.code, (t2.id), t3.t3
+ -> Seq Scan on public.emp1 t2
+ Output: t2.id, t2.code
+ Filter: (t2.id IS NOT NULL)
+ -> Function Scan on pg_catalog.generate_series t3
+ Output: t3.t3, t2.id
+ Function Call: generate_series(1, 1)
+(8 rows)
+
+explain (verbose, costs off)
+select * from generate_series(1,10) t1(id) left join
+ lateral (select t1.id as t1id, t2.id from emp1 t2 join emp1 t3 on t2.id = t3.id)
+on true;
+ QUERY PLAN
+------------------------------------------------------
+ Nested Loop Left Join
+ Output: t1.id, (t1.id), t3.id
+ -> Function Scan on pg_catalog.generate_series t1
+ Output: t1.id
+ Function Call: generate_series(1, 10)
+ -> Seq Scan on public.emp1 t3
+ Output: t3.id, t1.id
+ Filter: (t3.id IS NOT NULL)
+(8 rows)
-- We can remove the join even if we find the join can't duplicate rows and
-- the base quals of each side are different. In the following case we end up
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 8a8a63bd2f..9d6fce21de 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2600,12 +2600,16 @@ select * from emp1 t1 left join
on true)
on true;
--- Check that SJE does not remove self joins if a PHV references the removed
--- rel laterally.
-explain (costs off)
+-- Check that PHVs do not impose any constraints on removing self joins
+explain (verbose, costs off)
select * from emp1 t1 join emp1 t2 on t1.id = t2.id left join
lateral (select t1.id as t1id, * from generate_series(1,1) t3) s on true;
+explain (verbose, costs off)
+select * from generate_series(1,10) t1(id) left join
+ lateral (select t1.id as t1id, t2.id from emp1 t2 join emp1 t3 on t2.id = t3.id)
+on true;
+
-- We can remove the join even if we find the join can't duplicate rows and
-- the base quals of each side are different. In the following case we end up
-- moving quals over to s1 to make it so it can't match any rows.
--
2.31.0
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-22 02:32 Andrei Lepikhov <a.lepikhov@postgrespro.ru>
parent: Richard Guo <guofenglinux@gmail.com>
1 sibling, 1 reply; 24+ messages in thread
From: Andrei Lepikhov @ 2023-11-22 02:32 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; Alexander Korotkov <aekorotkov@gmail.com>; +Cc: zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On 20/11/2023 15:49, Richard Guo wrote:
> I'm wondering if we can relax this restriction because it seems to me
> that a PHV evaluated/needed at or below the self join should not have
> problem if we remove the self join.
>
>
> 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.
>
> Attached is a patch to remove such constraints. Any comments or
> feedback are welcome.
Carrying out excavations, I found that it was Teodor Sigaev who added
this limitation in v.20 of the patch in an earlier stage of the development.
I also had analyzed this part of code and came to the conclusion that it
doesn't needed. Only a paranoid approach and the absence of an
alternative opinion are the reasons why this code is still exists.
Your patch is ok.
I think the comment, 'PHVs should not impose ...' looks redundant. It
may be enough to have it in the commit comment.
--
regards,
Andrei Lepikhov
Postgres Professional
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-22 08:59 Richard Guo <guofenglinux@gmail.com>
parent: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 0 replies; 24+ messages in thread
From: Richard Guo @ 2023-11-22 08:59 UTC (permalink / raw)
To: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: Alexander Korotkov <aekorotkov@gmail.com>; zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Tom Lane <tgl@sss.pgh.pa.us>
On Wed, Nov 22, 2023 at 10:32 AM Andrei Lepikhov <a.lepikhov@postgrespro.ru>
wrote:
> Your patch is ok.
Thanks for looking into this patch.
> I think the comment, 'PHVs should not impose ...' looks redundant. It
> may be enough to have it in the commit comment.
Hmm, I'm not sure. I'm thinking that when people read this part of the
code, they might be confused as to why we don't consider PHVs. And this
comment will give them some clues. This is just like how we explain why
we need not worry about LATERAL nor PHVs in is_simple_values().
Thanks
Richard
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-27 17:42 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
1 sibling, 1 reply; 24+ messages in thread
From: Tom Lane @ 2023-11-27 17:42 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; Alexander Korotkov <aekorotkov@gmail.com>; zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
Richard Guo <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 = t2.id
) on i.f0 = t1.id;
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.
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)".
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.
BTW, why is it that it seems to prefer to remove the first of
the two self-joined rels, rather than the second? That seems
jarringly bizarre.
regards, tom lane
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-28 07:03 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 3 replies; 24+ messages in thread
From: Richard Guo @ 2023-11-28 07:03 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; Alexander Korotkov <aekorotkov@gmail.com>; zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On Tue, Nov 28, 2023 at 1:42 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Richard Guo <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 = t2.id
> ) on i.f0 = 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.
> BTW, why is it that it seems to prefer to remove the first of
> the two self-joined rels, rather than the second? That seems
> jarringly bizarre.
Hmm, I'm not sure either. Alexander and Andrei, could you please share
your insights?
[1]
https://www.postgresql.org/message-id/20231127180705.svzrlahdmqvosqfz%40awork3.anarazel.de
Thanks
Richard
Attachments:
v3-0001-Don-t-constrain-self-join-removal-due-to-PHVs.patch (application/octet-stream, 6.5K) download
From dd5ad58abc04beefde1ab3755f3d58451d879be0 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 20 Nov 2023 16:14:24 +0800
Subject: [PATCH v3] Don't constrain self join removal due to PHVs
---
src/backend/optimizer/plan/analyzejoins.c | 38 +++++++++++++---------
src/test/regress/expected/join.out | 39 ++++++++++++++++-------
src/test/regress/sql/join.sql | 10 ++++--
3 files changed, 58 insertions(+), 29 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index b9be19a687..9360177af8 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -453,7 +453,7 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
{
PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(l);
- Assert(!bms_is_member(relid, phinfo->ph_lateral));
+ Assert(sjinfo == NULL || !bms_is_member(relid, phinfo->ph_lateral));
if (bms_is_subset(phinfo->ph_needed, joinrelids) &&
bms_is_member(relid, phinfo->ph_eval_at) &&
!bms_is_member(ojrelid, phinfo->ph_eval_at))
@@ -472,6 +472,8 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
phinfo->ph_needed = replace_relid(phinfo->ph_needed, relid, subst);
phinfo->ph_needed = replace_relid(phinfo->ph_needed, ojrelid, subst);
/* ph_needed might or might not become empty */
+ phinfo->ph_lateral = replace_relid(phinfo->ph_lateral, relid, subst);
+ /* ph_lateral might or might not be empty */
phv->phrels = replace_relid(phv->phrels, relid, subst);
phv->phrels = replace_relid(phv->phrels, ojrelid, subst);
Assert(!bms_is_empty(phv->phrels));
@@ -1456,7 +1458,16 @@ replace_varno_walker(Node *node, ReplaceVarnoContext *ctx)
}
return false;
}
- if (IsA(node, RestrictInfo))
+ else if (IsA(node, PlaceHolderVar))
+ {
+ PlaceHolderVar *phv = (PlaceHolderVar *) node;
+
+ phv->phrels = replace_relid(phv->phrels, ctx->from, ctx->to);
+ phv->phnullingrels = replace_relid(phv->phnullingrels, ctx->from, ctx->to);
+
+ /* fall through to recurse into the placeholder's expression */
+ }
+ else if (IsA(node, RestrictInfo))
{
RestrictInfo *rinfo = (RestrictInfo *) node;
int relid = -1;
@@ -1658,6 +1669,15 @@ sje_walker(Node *node, ReplaceVarnoContext *ctx)
}
return false;
}
+ else if (IsA(node, PlaceHolderVar))
+ {
+ PlaceHolderVar *phv = (PlaceHolderVar *) node;
+
+ phv->phrels = replace_relid(phv->phrels, ctx->from, ctx->to);
+ phv->phnullingrels = replace_relid(phv->phnullingrels, ctx->from, ctx->to);
+
+ /* fall through to recurse into the placeholder's expression */
+ }
return expression_tree_walker(node, sje_walker, (void *) ctx);
}
@@ -2125,20 +2145,8 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
joinrelids = bms_add_member(joinrelids, k);
/*
- * Be safe to do not remove tables participated in complicated PH
+ * PHVs should not impose any constraints on removing self joins.
*/
- foreach(lc, root->placeholder_list)
- {
- PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(lc);
-
- /* there isn't any other place to eval PHV */
- if (bms_is_subset(phinfo->ph_eval_at, joinrelids) ||
- bms_is_subset(phinfo->ph_needed, joinrelids) ||
- bms_is_member(r, phinfo->ph_lateral))
- break;
- }
- if (lc)
- continue;
/*
* At this stage, joininfo lists of inner and outer can contain
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 2c73270143..69427287ff 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -6821,20 +6821,37 @@ on true;
Filter: (id IS NOT NULL)
(8 rows)
--- Check that SJE does not remove self joins if a PHV references the removed
--- rel laterally.
-explain (costs off)
+-- Check that PHVs do not impose any constraints on removing self joins
+explain (verbose, costs off)
select * from emp1 t1 join emp1 t2 on t1.id = t2.id left join
lateral (select t1.id as t1id, * from generate_series(1,1) t3) s on true;
- QUERY PLAN
----------------------------------------------------
+ QUERY PLAN
+----------------------------------------------------------
Nested Loop Left Join
- -> Nested Loop
- -> Seq Scan on emp1 t1
- -> Index Scan using emp1_pkey on emp1 t2
- Index Cond: (id = t1.id)
- -> Function Scan on generate_series t3
-(6 rows)
+ Output: t2.id, t2.code, t2.id, t2.code, (t2.id), t3.t3
+ -> Seq Scan on public.emp1 t2
+ Output: t2.id, t2.code
+ Filter: (t2.id IS NOT NULL)
+ -> Function Scan on pg_catalog.generate_series t3
+ Output: t3.t3, t2.id
+ Function Call: generate_series(1, 1)
+(8 rows)
+
+explain (verbose, costs off)
+select * from generate_series(1,10) t1(id) left join
+ lateral (select t1.id as t1id, t2.id from emp1 t2 join emp1 t3 on t2.id = t3.id)
+on true;
+ QUERY PLAN
+------------------------------------------------------
+ Nested Loop Left Join
+ Output: t1.id, (t1.id), t3.id
+ -> Function Scan on pg_catalog.generate_series t1
+ Output: t1.id
+ Function Call: generate_series(1, 10)
+ -> Seq Scan on public.emp1 t3
+ Output: t3.id, t1.id
+ Filter: (t3.id IS NOT NULL)
+(8 rows)
-- We can remove the join even if we find the join can't duplicate rows and
-- the base quals of each side are different. In the following case we end up
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 8a8a63bd2f..9d6fce21de 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2600,12 +2600,16 @@ select * from emp1 t1 left join
on true)
on true;
--- Check that SJE does not remove self joins if a PHV references the removed
--- rel laterally.
-explain (costs off)
+-- Check that PHVs do not impose any constraints on removing self joins
+explain (verbose, costs off)
select * from emp1 t1 join emp1 t2 on t1.id = t2.id left join
lateral (select t1.id as t1id, * from generate_series(1,1) t3) s on true;
+explain (verbose, costs off)
+select * from generate_series(1,10) t1(id) left join
+ lateral (select t1.id as t1id, t2.id from emp1 t2 join emp1 t3 on t2.id = t3.id)
+on true;
+
-- We can remove the join even if we find the join can't duplicate rows and
-- the base quals of each side are different. In the following case we end up
-- moving quals over to s1 to make it so it can't match any rows.
--
2.31.0
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-28 08:03 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
2 siblings, 1 reply; 24+ messages in thread
From: Richard Guo @ 2023-11-28 08:03 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; Alexander Korotkov <aekorotkov@gmail.com>; zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On Tue, Nov 28, 2023 at 3:03 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Tue, Nov 28, 2023 at 1:42 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> BTW, why is it that it seems to prefer to remove the first of
>> the two self-joined rels, rather than the second? That seems
>> jarringly bizarre.
>
>
> Hmm, I'm not sure either. Alexander and Andrei, could you please share
> your insights?
>
BTW, while reading the codes, I noticed this commit of
remove_self_joins_recurse.
* ... To avoid complexity, limit the max power of this set by a GUC.
But where is the GUC? I guess that it refers to
self_join_search_limit, which has been removed during development.
So we should revise this commit to at least remove any mention of the
GUC. Maybe it'd better to add a new commit explaining why we are not
concerned about cases where the number of self joins is too large.
Thanks
Richard
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-28 08:24 Andrei Lepikhov <a.lepikhov@postgrespro.ru>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 0 replies; 24+ messages in thread
From: Andrei Lepikhov @ 2023-11-28 08:24 UTC (permalink / raw)
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>
On 28/11/2023 15:03, Richard Guo wrote:
> On Tue, Nov 28, 2023 at 3:03 PM Richard Guo <guofenglinux@gmail.com
> <mailto:guofenglinux@gmail.com>> wrote:
> On Tue, Nov 28, 2023 at 1:42 AM Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.us>> wrote:
> BTW, why is it that it seems to prefer to remove the first of
> the two self-joined rels, rather than the second? That seems
> jarringly bizarre.
> Hmm, I'm not sure either. Alexander and Andrei, could you please share
> your insights?
I was thinking about that choice and, in earlier versions, removed outer
relations. I did not find a difference in removing a specific side of
the JOIN. Rewriting the patch right before the commit, I found out that
remove_useless_joins removed the inner join and chose to do the same,
like a convention.
> BTW, while reading the codes, I noticed this commit of
> remove_self_joins_recurse.
>
> * ... To avoid complexity, limit the max power of this set by a GUC.
>
> But where is the GUC? Â I guess that it refers to
> self_join_search_limit, which has been removed during development.
>
> So we should revise this commit to at least remove any mention of the
> GUC. Maybe it'd better to add a new commit explaining why we are not
> concerned about cases where the number of self joins is too large.
Maybe. But actually, we just did some benchmarking to see the influence.
Moreover, Alexander's code on preliminary sorting the relations and the
fact that inheritance isn't still expanded were reasons we aren't afraid
of significant degradation in practical cases.
--
regards,
Andrei Lepikhov
Postgres Professional
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-11-30 07:21 Andrei Lepikhov <a.lepikhov@postgrespro.ru>
parent: Richard Guo <guofenglinux@gmail.com>
2 siblings, 0 replies; 24+ messages in thread
From: Andrei Lepikhov @ 2023-11-30 07:21 UTC (permalink / raw)
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>
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
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-12-05 02:35 Richard Guo <guofenglinux@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
2 siblings, 1 reply; 24+ messages in thread
From: Richard Guo @ 2023-12-05 02:35 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; Alexander Korotkov <aekorotkov@gmail.com>; zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On Tue, Nov 28, 2023 at 3:03 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Tue, Nov 28, 2023 at 1:42 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> 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.
>
I noticed that this issue exists on master even without the
Don't-constrain-SJE-due-to-PHVs patch, and it can be illustrated with a
query from regression test.
-- Test that placeholders are updated correctly after join removal
explain (costs off)
select * from (values (1)) x
left join (select coalesce(y.q1, 1) from int8_tbl y
right join sj j1 inner join sj j2 on j1.a = j2.a
on true) z
on true;
In this query it successfully removes the self-join between j1 (6) and
j2 (7). But in the processed_tlist, you can still find PlaceHolderVar
with ":phrels (b 5 6 7 9)". So I think we need to handle PlaceHolderVar
case in sje_walker and replace_varno_walker, whether or not we
incorporate the Don't-constrain-SJE-due-to-PHVs patch.
Also, it seems to me that sje_walker is redundant. Currently it is used
to walk the query tree for varno replacement, a task that can be
fulfilled with replace_varno_walker.
0001 adds the handling of PlaceHolderVar case in replace_varno_walker,
and meanwhile retires sje_walker.
0002 is the original Don't-constrain-SJE-due-to-PHVs patch.
0003 is a fix of the comment for remove_self_joins_recurse, as described
upthread.
Thanks
Richard
Attachments: download all (3 files, .tar.gz)
v4-0002-Don-t-constrain-self-join-removal-due-to-PHVs.patch (application/octet-stream, 5.5K) download
From 12bcd568cff71f615a39ec46ec58051be07d69f0 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 20 Nov 2023 16:14:24 +0800
Subject: [PATCH v4 2/3] Don't constrain self join removal due to PHVs
---
src/backend/optimizer/plan/analyzejoins.c | 18 +++--------
src/test/regress/expected/join.out | 39 ++++++++++++++++-------
src/test/regress/sql/join.sql | 10 ++++--
3 files changed, 39 insertions(+), 28 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 9da2ffe325..083444c881 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -453,7 +453,7 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
{
PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(l);
- Assert(!bms_is_member(relid, phinfo->ph_lateral));
+ Assert(sjinfo == NULL || !bms_is_member(relid, phinfo->ph_lateral));
if (bms_is_subset(phinfo->ph_needed, joinrelids) &&
bms_is_member(relid, phinfo->ph_eval_at) &&
!bms_is_member(ojrelid, phinfo->ph_eval_at))
@@ -472,6 +472,8 @@ remove_rel_from_query(PlannerInfo *root, RelOptInfo *rel,
phinfo->ph_needed = replace_relid(phinfo->ph_needed, relid, subst);
phinfo->ph_needed = replace_relid(phinfo->ph_needed, ojrelid, subst);
/* ph_needed might or might not become empty */
+ phinfo->ph_lateral = replace_relid(phinfo->ph_lateral, relid, subst);
+ /* ph_lateral might or might not be empty */
phv->phrels = replace_relid(phv->phrels, relid, subst);
phv->phrels = replace_relid(phv->phrels, ojrelid, subst);
Assert(!bms_is_empty(phv->phrels));
@@ -2115,20 +2117,8 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
joinrelids = bms_add_member(joinrelids, k);
/*
- * Be safe to do not remove tables participated in complicated PH
+ * PHVs should not impose any constraints on removing self joins.
*/
- foreach(lc, root->placeholder_list)
- {
- PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(lc);
-
- /* there isn't any other place to eval PHV */
- if (bms_is_subset(phinfo->ph_eval_at, joinrelids) ||
- bms_is_subset(phinfo->ph_needed, joinrelids) ||
- bms_is_member(r, phinfo->ph_lateral))
- break;
- }
- if (lc)
- continue;
/*
* At this stage, joininfo lists of inner and outer can contain
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 2c73270143..69427287ff 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -6821,20 +6821,37 @@ on true;
Filter: (id IS NOT NULL)
(8 rows)
--- Check that SJE does not remove self joins if a PHV references the removed
--- rel laterally.
-explain (costs off)
+-- Check that PHVs do not impose any constraints on removing self joins
+explain (verbose, costs off)
select * from emp1 t1 join emp1 t2 on t1.id = t2.id left join
lateral (select t1.id as t1id, * from generate_series(1,1) t3) s on true;
- QUERY PLAN
----------------------------------------------------
+ QUERY PLAN
+----------------------------------------------------------
Nested Loop Left Join
- -> Nested Loop
- -> Seq Scan on emp1 t1
- -> Index Scan using emp1_pkey on emp1 t2
- Index Cond: (id = t1.id)
- -> Function Scan on generate_series t3
-(6 rows)
+ Output: t2.id, t2.code, t2.id, t2.code, (t2.id), t3.t3
+ -> Seq Scan on public.emp1 t2
+ Output: t2.id, t2.code
+ Filter: (t2.id IS NOT NULL)
+ -> Function Scan on pg_catalog.generate_series t3
+ Output: t3.t3, t2.id
+ Function Call: generate_series(1, 1)
+(8 rows)
+
+explain (verbose, costs off)
+select * from generate_series(1,10) t1(id) left join
+ lateral (select t1.id as t1id, t2.id from emp1 t2 join emp1 t3 on t2.id = t3.id)
+on true;
+ QUERY PLAN
+------------------------------------------------------
+ Nested Loop Left Join
+ Output: t1.id, (t1.id), t3.id
+ -> Function Scan on pg_catalog.generate_series t1
+ Output: t1.id
+ Function Call: generate_series(1, 10)
+ -> Seq Scan on public.emp1 t3
+ Output: t3.id, t1.id
+ Filter: (t3.id IS NOT NULL)
+(8 rows)
-- We can remove the join even if we find the join can't duplicate rows and
-- the base quals of each side are different. In the following case we end up
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 8a8a63bd2f..9d6fce21de 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2600,12 +2600,16 @@ select * from emp1 t1 left join
on true)
on true;
--- Check that SJE does not remove self joins if a PHV references the removed
--- rel laterally.
-explain (costs off)
+-- Check that PHVs do not impose any constraints on removing self joins
+explain (verbose, costs off)
select * from emp1 t1 join emp1 t2 on t1.id = t2.id left join
lateral (select t1.id as t1id, * from generate_series(1,1) t3) s on true;
+explain (verbose, costs off)
+select * from generate_series(1,10) t1(id) left join
+ lateral (select t1.id as t1id, t2.id from emp1 t2 join emp1 t3 on t2.id = t3.id)
+on true;
+
-- We can remove the join even if we find the join can't duplicate rows and
-- the base quals of each side are different. In the following case we end up
-- moving quals over to s1 to make it so it can't match any rows.
--
2.31.0
v4-0001-Handle-PlaceHolderVar-case-in-replace_varno_walker.patch (application/octet-stream, 2.2K) download
From c05269a4beb711ff5fd547625288e0709bfd8732 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Tue, 5 Dec 2023 10:16:33 +0800
Subject: [PATCH v4 1/3] Handle PlaceHolderVar case in replace_varno_walker
This patch also retires sje_walker.
---
src/backend/optimizer/plan/analyzejoins.c | 34 ++++++++---------------
1 file changed, 12 insertions(+), 22 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index b9be19a687..9da2ffe325 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -1456,7 +1456,16 @@ replace_varno_walker(Node *node, ReplaceVarnoContext *ctx)
}
return false;
}
- if (IsA(node, RestrictInfo))
+ else if (IsA(node, PlaceHolderVar))
+ {
+ PlaceHolderVar *phv = (PlaceHolderVar *) node;
+
+ phv->phrels = replace_relid(phv->phrels, ctx->from, ctx->to);
+ phv->phnullingrels = replace_relid(phv->phnullingrels, ctx->from, ctx->to);
+
+ /* fall through to recurse into the placeholder's expression */
+ }
+ else if (IsA(node, RestrictInfo))
{
RestrictInfo *rinfo = (RestrictInfo *) node;
int relid = -1;
@@ -1641,26 +1650,6 @@ update_eclasses(EquivalenceClass *ec, int from, int to)
ec->ec_relids = replace_relid(ec->ec_relids, from, to);
}
-static bool
-sje_walker(Node *node, ReplaceVarnoContext *ctx)
-{
- if (node == NULL)
- return false;
-
- if (IsA(node, Var))
- {
- Var *var = (Var *) node;
-
- if (var->varno == ctx->from)
- {
- var->varno = ctx->to;
- var->varnosyn = ctx->to;
- }
- return false;
- }
- return expression_tree_walker(node, sje_walker, (void *) ctx);
-}
-
/*
* Remove a relation after we have proven that it participates only in an
* unneeded unique self join.
@@ -1868,7 +1857,8 @@ remove_self_join_rel(PlannerInfo *root, PlanRowMark *kmark, PlanRowMark *rmark,
}
/* Replace varno in all the query structures */
- query_tree_walker(root->parse, sje_walker, &ctx, QTW_EXAMINE_SORTGROUP);
+ query_tree_walker(root->parse, replace_varno_walker, &ctx,
+ QTW_EXAMINE_SORTGROUP);
/* Replace links in the planner info */
remove_rel_from_query(root, toRemove, toKeep->relid, NULL, NULL);
--
2.31.0
v4-0003-Fix-a-comment-for-remove_self_joins_recurse.patch (application/octet-stream, 920B) download
From d9961a7137b7109ead8386302a56df3c19da5bd8 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Tue, 5 Dec 2023 10:19:52 +0800
Subject: [PATCH v4 3/3] Fix a comment for remove_self_joins_recurse
---
src/backend/optimizer/plan/analyzejoins.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 083444c881..ec761d1e74 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -2186,7 +2186,7 @@ remove_self_joins_one_group(PlannerInfo *root, Relids relids)
/*
* Gather indexes of base relations from the joinlist and try to eliminate self
- * joins. To avoid complexity, limit the max power of this set by a GUC.
+ * joins.
*/
static Relids
remove_self_joins_recurse(PlannerInfo *root, List *joinlist, Relids toRemove)
--
2.31.0
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-12-07 02:55 Andrei Lepikhov <a.lepikhov@postgrespro.ru>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Andrei Lepikhov @ 2023-12-07 02:55 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; Alexander Korotkov <aekorotkov@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; +Cc: zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On Tue, Nov 28, 2023 at 1:42 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> :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.
I must confess I couldn't reproduce this issue. I watched only the value
':phnullingrels (b)' everywhere.
On 5/12/2023 09:35, Richard Guo wrote:
>
> On Tue, Nov 28, 2023 at 3:03 PM Richard Guo <guofenglinux@gmail.com
> <mailto:guofenglinux@gmail.com>> wrote:
>
> On Tue, Nov 28, 2023 at 1:42 AM Tom Lane <tgl@sss.pgh.pa.us
> <mailto:tgl@sss.pgh.pa.us>> wrote:
>
> 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.
Yes, it was my blunder. I didn't recognize that phinfo->ph_var points to
a copy of the PlaceHolderVar and thought it was enough to pass through
the root->placeholder_list only. Adding replacement code into the walker
is the proper correction.
> I noticed that this issue exists on master even without the
> Don't-constrain-SJE-due-to-PHVs patch, and it can be illustrated with a
> query from regression test.
>
> -- Test that placeholders are updated correctly after join removal
> explain (costs off)
> select * from (values (1)) x
> left join (select coalesce(y.q1, 1) from int8_tbl y
> Â Â right join sj j1 inner join sj j2 on j1.a = j2.a
> Â Â on true) z
> on true;
> Also, it seems to me that sje_walker is redundant. Currently it is used
> to walk the query tree for varno replacement, a task that can be
> fulfilled with replace_varno_walker.
Agree.
>> 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 separate patch.
It is an excellent task to ponder. At least, we should answer later why
the setrefs.c didn't detect that, and could we do something here. Maybe
all we could do - is add comments to the PlannerInfo structure.
According to the patches:
0001 - looks good for me.
0002 - I don't understand why to use 'explain' in VERBOSE mode in tests.
What do you try to detect here?
0003 - ok, but too short. Maybe squash all these patches into one?
I think these corrections are good enough for commit.
--
regards,
Andrei Lepikhov
Postgres Professional
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-12-12 06:41 Richard Guo <guofenglinux@gmail.com>
parent: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 1 reply; 24+ messages in thread
From: Richard Guo @ 2023-12-12 06:41 UTC (permalink / raw)
To: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: Alexander Korotkov <aekorotkov@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On Thu, Dec 7, 2023 at 10:55 AM Andrei Lepikhov <a.lepikhov@postgrespro.ru>
wrote:
> According to the patches:
> 0001 - looks good for me.
> 0002 - I don't understand why to use 'explain' in VERBOSE mode in tests.
> What do you try to detect here?
The VERBOSE mode is used to observe that PHVs are adjusted correctly and
put in the right targetlist.
> 0003 - ok, but too short. Maybe squash all these patches into one?
It seems to me that these three patches are addressing separate issues
and are independent of each other. For instance, the issue addressed by
0001 exists on master even without other changes, and 0002 is an
improvement that does not depend on others, while 0003 is a fix of an
incorrect comment. I don't think it's a good idea to squash them into
one.
> I think these corrections are good enough for commit.
Thank you for reviewing these patches. Alexander, what are your
thoughts on these patches?
Thanks
Richard
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-12-22 23:27 Alexander Korotkov <aekorotkov@gmail.com>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Alexander Korotkov @ 2023-12-22 23:27 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; Tom Lane <tgl@sss.pgh.pa.us>; zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On Tue, Dec 12, 2023 at 8:41 AM Richard Guo <guofenglinux@gmail.com> wrote:
> On Thu, Dec 7, 2023 at 10:55 AM Andrei Lepikhov <a.lepikhov@postgrespro.ru> wrote:
>> According to the patches:
>> 0001 - looks good for me.
>> 0002 - I don't understand why to use 'explain' in VERBOSE mode in tests.
>> What do you try to detect here?
>
> The VERBOSE mode is used to observe that PHVs are adjusted correctly and
> put in the right targetlist.
>
>> 0003 - ok, but too short. Maybe squash all these patches into one?
>
> It seems to me that these three patches are addressing separate issues
> and are independent of each other. For instance, the issue addressed by
> 0001 exists on master even without other changes, and 0002 is an
> improvement that does not depend on others, while 0003 is a fix of an
> incorrect comment. I don't think it's a good idea to squash them into
> one.
>
>> I think these corrections are good enough for commit.
>
> Thank you for reviewing these patches. Alexander, what are your
> thoughts on these patches?
Looks good to me. I'm going to push this if there are no objections.
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-12-24 23:37 Alexander Korotkov <aekorotkov@gmail.com>
parent: Alexander Korotkov <aekorotkov@gmail.com>
0 siblings, 1 reply; 24+ messages in thread
From: Alexander Korotkov @ 2023-12-24 23:37 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; Tom Lane <tgl@sss.pgh.pa.us>; zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On Sat, Dec 23, 2023 at 1:27 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Tue, Dec 12, 2023 at 8:41 AM Richard Guo <guofenglinux@gmail.com> wrote:
> > On Thu, Dec 7, 2023 at 10:55 AM Andrei Lepikhov <a.lepikhov@postgrespro.ru> wrote:
> >> According to the patches:
> >> 0001 - looks good for me.
> >> 0002 - I don't understand why to use 'explain' in VERBOSE mode in tests.
> >> What do you try to detect here?
> >
> > The VERBOSE mode is used to observe that PHVs are adjusted correctly and
> > put in the right targetlist.
> >
> >> 0003 - ok, but too short. Maybe squash all these patches into one?
> >
> > It seems to me that these three patches are addressing separate issues
> > and are independent of each other. For instance, the issue addressed by
> > 0001 exists on master even without other changes, and 0002 is an
> > improvement that does not depend on others, while 0003 is a fix of an
> > incorrect comment. I don't think it's a good idea to squash them into
> > one.
> >
> >> I think these corrections are good enough for commit.
> >
> > Thank you for reviewing these patches. Alexander, what are your
> > thoughts on these patches?
>
> Looks good to me. I'm going to push this if there are no objections.
Pushed! Thanks to all the participants.
------
Regards,
Alexander Korotkov
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN
@ 2023-12-25 07:18 Richard Guo <guofenglinux@gmail.com>
parent: Alexander Korotkov <aekorotkov@gmail.com>
0 siblings, 0 replies; 24+ messages in thread
From: Richard Guo @ 2023-12-25 07:18 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Andrei Lepikhov <a.lepikhov@postgrespro.ru>; Tom Lane <tgl@sss.pgh.pa.us>; zuming.jiang@inf.ethz.ch, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>
On Mon, Dec 25, 2023 at 7:38 AM Alexander Korotkov <aekorotkov@gmail.com>
wrote:
> On Sat, Dec 23, 2023 at 1:27 AM Alexander Korotkov <aekorotkov@gmail.com>
> wrote:
> > On Tue, Dec 12, 2023 at 8:41 AM Richard Guo <guofenglinux@gmail.com>
> wrote:
> > > Thank you for reviewing these patches. Alexander, what are your
> > > thoughts on these patches?
> >
> > Looks good to me. I'm going to push this if there are no objections.
>
> Pushed! Thanks to all the participants.
Thanks for pushing!
Thanks
Richard
^ permalink raw reply [nested|flat] 24+ messages in thread
end of thread, other threads:[~2023-12-25 07:18 UTC | newest]
Thread overview: 24+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-11-07 17:04 BUG #18187: Unexpected error: "variable not found in subplan target lists" triggered by JOIN PG Bug reporting form <noreply@postgresql.org>
2023-11-08 15:42 ` Andrei Lepikhov <a.lepikhov@postgrespro.ru>
2023-11-08 17:18 ` Zu-Ming Jiang <zuming.jiang@inf.ethz.ch>
2023-11-09 03:54 ` Richard Guo <guofenglinux@gmail.com>
2023-11-09 04:51 ` Andrei Lepikhov <a.lepikhov@postgrespro.ru>
2023-11-09 11:08 ` Richard Guo <guofenglinux@gmail.com>
2023-11-09 11:21 ` Andrei Lepikhov <a.lepikhov@postgrespro.ru>
2023-11-09 12:25 ` Alexander Korotkov <aekorotkov@gmail.com>
2023-11-10 08:20 ` Richard Guo <guofenglinux@gmail.com>
2023-11-10 21:02 ` Alexander Korotkov <aekorotkov@gmail.com>
2023-11-20 08:49 ` Richard Guo <guofenglinux@gmail.com>
2023-11-22 02:32 ` Andrei Lepikhov <a.lepikhov@postgrespro.ru>
2023-11-22 08:59 ` Richard Guo <guofenglinux@gmail.com>
2023-11-27 17:42 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-11-28 07:03 ` Richard Guo <guofenglinux@gmail.com>
2023-11-28 08:03 ` Richard Guo <guofenglinux@gmail.com>
2023-11-28 08:24 ` Andrei Lepikhov <a.lepikhov@postgrespro.ru>
2023-11-30 07:21 ` Andrei Lepikhov <a.lepikhov@postgrespro.ru>
2023-12-05 02:35 ` Richard Guo <guofenglinux@gmail.com>
2023-12-07 02:55 ` Andrei Lepikhov <a.lepikhov@postgrespro.ru>
2023-12-12 06:41 ` Richard Guo <guofenglinux@gmail.com>
2023-12-22 23:27 ` Alexander Korotkov <aekorotkov@gmail.com>
2023-12-24 23:37 ` Alexander Korotkov <aekorotkov@gmail.com>
2023-12-25 07:18 ` Richard Guo <guofenglinux@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox