Make replace_relid() leave argument unmodified

There are a lot of situations when we share the same pointer to a Bitmapset
structure across different places.  In order to evade undesirable side effects
replace_relid() function should always return a copy.

Reported-by: Richard Guo
Discussion: https://postgr.es/m/CAMbWs4_wJthNtYBL%2BSsebpgF-5L2r5zFFk6xYbS0A78GKOTFHw%40mail.gmail.com
Reviewed-by: Richard Guo, Andres Freund, Ashutosh Bapat, Andrei Lepikhov

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 6c02fe89085ac7f67be2388e9904af3ee98b3f17..80739451b7c74b7d655ea6fdd186cef57b7c093a 100644 (file)
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -1522,6 +1522,10 @@ replace_varno_walker(Node *node, ReplaceVarnoContext *ctx)
 
 /*
  * Substitute newId by oldId in relids.
+ *
+ * We must make a copy of the original Bitmapset before making any
+ * modifications, because the same pointer to it might be shared among
+ * different places.
  */
 static Bitmapset *
 replace_relid(Relids relids, int oldId, int newId)
@@ -1529,12 +1533,13 @@ replace_relid(Relids relids, int oldId, int newId)
    if (oldId < 0)
        return relids;
 
+   /* Delete relid without substitution. */
    if (newId < 0)
-       /* Delete relid without substitution. */
-       return bms_del_member(relids, oldId);
+       return bms_del_member(bms_copy(relids), oldId);
 
+   /* Substitute newId for oldId. */
    if (bms_is_member(oldId, relids))
-       return bms_add_member(bms_del_member(relids, oldId), newId);
+       return bms_add_member(bms_del_member(bms_copy(relids), oldId), newId);
 
    return relids;
 }

diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 69427287ffdc60eaab498db32c2df6adbdc27a11..1557e17299c64d1ca075a962b0443f4caf409a76 100644 (file)
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -6853,6 +6853,21 @@ on true;
          Filter: (t3.id IS NOT NULL)
 (8 rows)
 
+-- Check that SJE replaces join clauses involving the removed rel correctly
+explain (costs off)
+select * from emp1 t1
+   inner join emp1 t2 on t1.id = t2.id
+    left join emp1 t3 on t1.id > 1 and t1.id < 2;
+                  QUERY PLAN                  
+----------------------------------------------
+ Nested Loop Left Join
+   Join Filter: ((t2.id > 1) AND (t2.id < 2))
+   ->  Seq Scan on emp1 t2
+         Filter: (id IS NOT NULL)
+   ->  Materialize
+         ->  Seq Scan on emp1 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 9d6fce21ded3736731b3c0ae7b155bc046e8e937..fed9e83e31d373a5ceb0ec763c566d93e92e1c56 100644 (file)
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -2610,6 +2610,12 @@ 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;
 
+-- Check that SJE replaces join clauses involving the removed rel correctly
+explain (costs off)
+select * from emp1 t1
+   inner join emp1 t2 on t1.id = t2.id
+    left join emp1 t3 on t1.id > 1 and t1.id < 2;
+
 -- 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.