From c197405fbae7f6b8abe217fbfab8c4882a872229 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Fri, 24 Nov 2023 14:57:27 +0200 Subject: [PATCH 3/3] 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. --- src/backend/optimizer/plan/analyzejoins.c | 11 ++++++++--- src/test/regress/expected/join.out | 15 +++++++++++++++ src/test/regress/sql/join.sql | 6 ++++++ 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c index b9be19a687f..75e4ebc64a2 100644 --- a/src/backend/optimizer/plan/analyzejoins.c +++ b/src/backend/optimizer/plan/analyzejoins.c @@ -1511,6 +1511,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) @@ -1518,12 +1522,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 2c73270143b..5ceecc44132 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -6836,6 +6836,21 @@ select * from emp1 t1 join emp1 t2 on t1.id = t2.id left join -> Function Scan on generate_series t3 (6 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 8a8a63bd2f1..c3ec340e2da 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2606,6 +2606,12 @@ 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; +-- 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. -- 2.39.3 (Apple Git-145)