From 187ae17300776f48b2bd9d0737923b1bf70f606e Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Mon, 9 Jun 2014 21:28:56 -0400
Subject: [PATCH] Fix planner bug with nested PlaceHolderVars in 9.2 (only).
Commit 9e7e29c75ad441450f9b8287bd51c13521641e3b fixed some problems with
LATERAL references in PlaceHolderVars, one of which was that "createplan.c
wasn't handling nested PlaceHolderVars properly". I failed to see that
this problem might occur in older versions as well; but it can, as
demonstrated in bug #10587 from Geoff Speicher. In this case the nesting
occurs due to push-down of PlaceHolderVar expressions into a parameterized
path. So, back-patch the relevant changes from 9e7e29c75ad4 into 9.2 where
parameterized paths were introduced. (Perhaps I'm still being too myopic,
but I'm hesitant to change older branches without some evidence that the
case can occur there.)
---
src/backend/nodes/equalfuncs.c | 16 ++++--
src/backend/optimizer/plan/createplan.c | 39 ++++++++++---
src/test/regress/expected/join.out | 74 +++++++++++++++++++++++++
src/test/regress/sql/join.sql | 58 +++++++++++++++++++
4 files changed, 172 insertions(+), 15 deletions(-)
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index dad859aedc6..36e9992de69 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -759,15 +759,19 @@ _equalPlaceHolderVar(const PlaceHolderVar *a, const PlaceHolderVar *b)
/*
* We intentionally do not compare phexpr. Two PlaceHolderVars with the
* same ID and levelsup should be considered equal even if the contained
- * expressions have managed to mutate to different states. One way in
- * which that can happen is that initplan sublinks would get replaced by
- * differently-numbered Params when sublink folding is done. (The end
- * result of such a situation would be some unreferenced initplans, which
- * is annoying but not really a problem.)
+ * expressions have managed to mutate to different states. This will
+ * happen during final plan construction when there are nested PHVs, since
+ * the inner PHV will get replaced by a Param in some copies of the outer
+ * PHV. Another way in which it can happen is that initplan sublinks
+ * could get replaced by differently-numbered Params when sublink folding
+ * is done. (The end result of such a situation would be some
+ * unreferenced initplans, which is annoying but not really a problem.) On
+ * the same reasoning, there is no need to examine phrels.
*
* COMPARE_NODE_FIELD(phexpr);
+ *
+ * COMPARE_BITMAPSET_FIELD(phrels);
*/
- COMPARE_BITMAPSET_FIELD(phrels);
COMPARE_SCALAR_FIELD(phid);
COMPARE_SCALAR_FIELD(phlevelsup);
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index 36997be5889..c5df505d491 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -2532,16 +2532,37 @@ replace_nestloop_params_mutator(Node *node, PlannerInfo *root)
Assert(phv->phlevelsup == 0);
/*
- * If not to be replaced, just return the PlaceHolderVar unmodified.
- * We use bms_overlap as a cheap/quick test to see if the PHV might be
- * evaluated in the outer rels, and then grab its PlaceHolderInfo to
- * tell for sure.
+ * Check whether we need to replace the PHV. We use bms_overlap as a
+ * cheap/quick test to see if the PHV might be evaluated in the outer
+ * rels, and then grab its PlaceHolderInfo to tell for sure.
*/
- if (!bms_overlap(phv->phrels, root->curOuterRels))
- return node;
- if (!bms_is_subset(find_placeholder_info(root, phv, false)->ph_eval_at,
- root->curOuterRels))
- return node;
+ if (!bms_overlap(phv->phrels, root->curOuterRels) ||
+ !bms_is_subset(find_placeholder_info(root, phv, false)->ph_eval_at,
+ root->curOuterRels))
+ {
+ /*
+ * We can't replace the whole PHV, but we might still need to
+ * replace Vars or PHVs within its expression, in case it ends up
+ * actually getting evaluated here. (It might get evaluated in
+ * this plan node, or some child node; in the latter case we don't
+ * really need to process the expression here, but we haven't got
+ * enough info to tell if that's the case.) Flat-copy the PHV
+ * node and then recurse on its expression.
+ *
+ * Note that after doing this, we might have different
+ * representations of the contents of the same PHV in different
+ * parts of the plan tree. This is OK because equal() will just
+ * match on phid/phlevelsup, so setrefs.c will still recognize an
+ * upper-level reference to a lower-level copy of the same PHV.
+ */
+ PlaceHolderVar *newphv = makeNode(PlaceHolderVar);
+
+ memcpy(newphv, phv, sizeof(PlaceHolderVar));
+ newphv->phexpr = (Expr *)
+ replace_nestloop_params_mutator((Node *) phv->phexpr,
+ root);
+ return (Node *) newphv;
+ }
/* Create a Param representing the PlaceHolderVar */
param = assign_nestloop_param_placeholdervar(root, phv);
/* Is this param already listed in root->curOuterParams? */
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index e292ef05f6b..7b550c31bab 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -2570,6 +2570,80 @@ SELECT qq, unique1
456 | 7318
(3 rows)
+--
+-- nested nestloops can require nested PlaceHolderVars
+--
+create temp table nt1 (
+ id int primary key,
+ a1 boolean,
+ a2 boolean
+);
+NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "nt1_pkey" for table "nt1"
+create temp table nt2 (
+ id int primary key,
+ nt1_id int,
+ b1 boolean,
+ b2 boolean,
+ foreign key (nt1_id) references nt1(id)
+);
+NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "nt2_pkey" for table "nt2"
+create temp table nt3 (
+ id int primary key,
+ nt2_id int,
+ c1 boolean,
+ foreign key (nt2_id) references nt2(id)
+);
+NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "nt3_pkey" for table "nt3"
+insert into nt1 values (1,true,true);
+insert into nt1 values (2,true,false);
+insert into nt1 values (3,false,false);
+insert into nt2 values (1,1,true,true);
+insert into nt2 values (2,2,true,false);
+insert into nt2 values (3,3,false,false);
+insert into nt3 values (1,1,true);
+insert into nt3 values (2,2,false);
+insert into nt3 values (3,3,true);
+explain (costs off)
+select nt3.id
+from nt3 as nt3
+ left join
+ (select nt2.*, (nt2.b1 and ss1.a3) AS b3
+ from nt2 as nt2
+ left join
+ (select nt1.*, (nt1.id is not null) as a3 from nt1) as ss1
+ on ss1.id = nt2.nt1_id
+ ) as ss2
+ on ss2.id = nt3.nt2_id
+where nt3.id = 1 and ss2.b3;
+ QUERY PLAN
+-----------------------------------------------
+ Nested Loop
+ -> Nested Loop
+ -> Index Scan using nt3_pkey on nt3
+ Index Cond: (id = 1)
+ -> Index Scan using nt2_pkey on nt2
+ Index Cond: (id = nt3.nt2_id)
+ -> Index Only Scan using nt1_pkey on nt1
+ Index Cond: (id = nt2.nt1_id)
+ Filter: (nt2.b1 AND (id IS NOT NULL))
+(9 rows)
+
+select nt3.id
+from nt3 as nt3
+ left join
+ (select nt2.*, (nt2.b1 and ss1.a3) AS b3
+ from nt2 as nt2
+ left join
+ (select nt1.*, (nt1.id is not null) as a3 from nt1) as ss1
+ on ss1.id = nt2.nt1_id
+ ) as ss2
+ on ss2.id = nt3.nt2_id
+where nt3.id = 1 and ss2.b3;
+ id
+----
+ 1
+(1 row)
+
--
-- test case where a PlaceHolderVar is propagated into a subquery
--
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index 316a6e93d71..5834d3dfdcc 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -660,6 +660,64 @@ SELECT qq, unique1
USING (qq)
INNER JOIN tenk1 c ON qq = unique2;
+--
+-- nested nestloops can require nested PlaceHolderVars
+--
+
+create temp table nt1 (
+ id int primary key,
+ a1 boolean,
+ a2 boolean
+);
+create temp table nt2 (
+ id int primary key,
+ nt1_id int,
+ b1 boolean,
+ b2 boolean,
+ foreign key (nt1_id) references nt1(id)
+);
+create temp table nt3 (
+ id int primary key,
+ nt2_id int,
+ c1 boolean,
+ foreign key (nt2_id) references nt2(id)
+);
+
+insert into nt1 values (1,true,true);
+insert into nt1 values (2,true,false);
+insert into nt1 values (3,false,false);
+insert into nt2 values (1,1,true,true);
+insert into nt2 values (2,2,true,false);
+insert into nt2 values (3,3,false,false);
+insert into nt3 values (1,1,true);
+insert into nt3 values (2,2,false);
+insert into nt3 values (3,3,true);
+
+explain (costs off)
+select nt3.id
+from nt3 as nt3
+ left join
+ (select nt2.*, (nt2.b1 and ss1.a3) AS b3
+ from nt2 as nt2
+ left join
+ (select nt1.*, (nt1.id is not null) as a3 from nt1) as ss1
+ on ss1.id = nt2.nt1_id
+ ) as ss2
+ on ss2.id = nt3.nt2_id
+where nt3.id = 1 and ss2.b3;
+
+select nt3.id
+from nt3 as nt3
+ left join
+ (select nt2.*, (nt2.b1 and ss1.a3) AS b3
+ from nt2 as nt2
+ left join
+ (select nt1.*, (nt1.id is not null) as a3 from nt1) as ss1
+ on ss1.id = nt2.nt1_id
+ ) as ss2
+ on ss2.id = nt3.nt2_id
+where nt3.id = 1 and ss2.b3;
+
--
-- test case where a PlaceHolderVar is propagated into a subquery
--
--
GitLab