public inbox for [email protected]
help / color / mirror / Atom feedFrom: Richard Guo <[email protected]>
To: PostgreSQL-development <[email protected]>
Cc: Alexander Korotkov <[email protected]>
Subject: Assert failure on 'list_member_ptr(rel->joininfo, restrictinfo)'
Date: Tue, 14 Nov 2023 19:14:57 +0800
Message-ID: <CAMbWs4_wJthNtYBL+SsebpgF-5L2r5zFFk6xYbS0A78GKOTFHw@mail.gmail.com> (raw)
While working on BUG #18187 [1], I noticed that we also have issues with
how SJE replaces join clauses involving the removed rel. As an example,
consider the query below, which would trigger an Assert.
create table t (a int primary key, b int);
explain (costs off)
select * from t t1
inner join t t2 on t1.a = t2.a
left join t t3 on t1.b > 1 and t1.b < 2;
server closed the connection unexpectedly
The Assert failure happens in remove_self_join_rel() when we're trying
to remove t1. The two join clauses of t1, 't1.b > 1' and 't1.b < 2',
share the same pointer of 'required_relids', which is {t1, t3} at first.
After we've performed replace_varno for the first clause, the
required_relids becomes {t2, t3}, which is no problem. However, the
second clause's required_relids also becomes {t2, t3}, because they are
actually the same pointer. So when we proceed with replace_varno on the
second clause, we'd trigger the Assert.
Off the top of my head I'm thinking that we can fix this kind of issue
by bms_copying the bitmapset first before we make a substitution in
replace_relid(), like attached.
Alternatively, we can revise distribute_qual_to_rels() as below so that
different RestrictInfos don't share the same pointer of required_relids.
--- a/src/backend/optimizer/plan/initsplan.c
+++ b/src/backend/optimizer/plan/initsplan.c
@@ -2385,7 +2385,7 @@ distribute_qual_to_rels(PlannerInfo *root, Node
*clause,
* nonnullable-side rows failing the qual.
*/
Assert(ojscope);
- relids = ojscope;
+ relids = bms_copy(ojscope);
Assert(!pseudoconstant);
}
else
With this way, I'm worrying that there are other places where we should
avoid sharing the same pointer to Bitmapset structure. I'm not sure how
to discover all these places.
Any thoughts?
[1]
https://www.postgresql.org/message-id/flat/18187-831da249cbd2ff8e%40postgresql.org
Thanks
Richard
Attachments:
[application/octet-stream] v1-0001-Fix-how-SJE-replaces-join-clauses.patch (3.2K, ../CAMbWs4_wJthNtYBL+SsebpgF-5L2r5zFFk6xYbS0A78GKOTFHw@mail.gmail.com/3-v1-0001-Fix-how-SJE-replaces-join-clauses.patch)
download | inline diff:
From 8669edb991f4cb9d48fe8f85bec86636eef24652 Mon Sep 17 00:00:00 2001
From: Richard Guo <[email protected]>
Date: Tue, 14 Nov 2023 18:55:24 +0800
Subject: [PATCH v1] Fix how SJE replaces join clauses
---
src/backend/optimizer/plan/analyzejoins.c | 9 +++++++--
src/test/regress/expected/join.out | 15 +++++++++++++++
src/test/regress/sql/join.sql | 6 ++++++
3 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index b9be19a687..f94449df94 100644
--- a/src/backend/optimizer/plan/analyzejoins.c
+++ b/src/backend/optimizer/plan/analyzejoins.c
@@ -1518,12 +1518,17 @@ 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);
+ /*
+ * Substitute newId for oldId. We must make a copy of the original relids
+ * before starting the substitution, because the same pointer to a
+ * Bitmapset structure might be shared among different places.
+ */
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 2c73270143..5ceecc4413 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 8a8a63bd2f..c3ec340e2d 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.31.0
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected]
Subject: Re: Assert failure on 'list_member_ptr(rel->joininfo, restrictinfo)'
In-Reply-To: <CAMbWs4_wJthNtYBL+SsebpgF-5L2r5zFFk6xYbS0A78GKOTFHw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox