From 8c7bb02409548a88fa02d6f8771e2a6ca27007d3 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Thu, 6 Aug 2015 22:14:07 -0400
Subject: [PATCH] Fix old oversight in join removal logic.
Commit 9e7e29c75ad441450f9b8287bd51c13521641e3b introduced an Assert that
join removal didn't reduce the eval_at set of any PlaceHolderVar to empty.
At first glance it looks like join_is_removable ensures that's true --- but
actually, the loop in join_is_removable skips PlaceHolderVars that are not
referenced above the join due to be removed. So, if we don't want any
empty eval_at sets, the right thing to do is to delete any now-unreferenced
PlaceHolderVars from the data structure entirely.
Per fuzz testing by Andreas Seltenreich. Back-patch to 9.3 where the
aforesaid Assert was added.
---
src/backend/optimizer/plan/analyzejoins.c | 20 ++++++++++++++------
src/test/regress/expected/join.out | 16 ++++++++++++++++
src/test/regress/sql/join.sql | 15 +++++++++++++++
3 files changed, 45 insertions(+), 6 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 129fc3dfae6..6b967df3779 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -382,16 +382,24 @@ remove_rel_from_query(PlannerInfo *root, int relid, Relids joinrelids)
}
/*
- * Likewise remove references from PlaceHolderVar data structures.
+ * Likewise remove references from PlaceHolderVar data structures,
+ * removing any no-longer-needed placeholders entirely.
*/
- foreach(l, root->placeholder_list)
+ for (l = list_head(root->placeholder_list); l != NULL; l = nextl)
{
PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(l);
- phinfo->ph_eval_at = bms_del_member(phinfo->ph_eval_at, relid);
- Assert(!bms_is_empty(phinfo->ph_eval_at));
- Assert(!bms_is_member(relid, phinfo->ph_lateral));
- phinfo->ph_needed = bms_del_member(phinfo->ph_needed, relid);
+ nextl = lnext(l);
+ if (bms_is_subset(phinfo->ph_needed, joinrelids))
+ root->placeholder_list = list_delete_ptr(root->placeholder_list,
+ phinfo);
+ else
+ {
+ phinfo->ph_eval_at = bms_del_member(phinfo->ph_eval_at, relid);
+ Assert(!bms_is_empty(phinfo->ph_eval_at));
+ Assert(!bms_is_member(relid, phinfo->ph_lateral));
+ phinfo->ph_needed = bms_del_member(phinfo->ph_needed, relid);
+ }
}
/*
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 6c3d8fd7a04..68e688b9d3c 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -3743,6 +3743,22 @@ SELECT * FROM
1 | 4567890123456789 | -4567890123456789 | 4567890123456789
(5 rows)
+rollback;
+-- another join removal bug: we must clean up correctly when removing a PHV
+begin;
+create temp table uniquetbl (f1 text unique);
+explain (costs off)
+select t1.* from
+ uniquetbl as t1
+ left join (select *, '***'::text as d1 from uniquetbl) t2
+ on t1.f1 = t2.f1
+ left join uniquetbl t3
+ on t2.d1 = t3.f1;
+ QUERY PLAN
+--------------------------
+ Seq Scan on uniquetbl t1
+(1 row)
+
rollback;
-- bug #8444: we've historically allowed duplicate aliases within aliased JOINs
select * from
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 992e673c12e..402ba61eac7 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -1196,6 +1196,21 @@ SELECT * FROM
rollback;
+-- another join removal bug: we must clean up correctly when removing a PHV
+begin;
+
+create temp table uniquetbl (f1 text unique);
+
+explain (costs off)
+select t1.* from
+ uniquetbl as t1
+ left join (select *, '***'::text as d1 from uniquetbl) t2
+ on t1.f1 = t2.f1
+ left join uniquetbl t3
+ on t2.d1 = t3.f1;
+
+rollback;
+
-- bug #8444: we've historically allowed duplicate aliases within aliased JOINs
select * from
--
GitLab