From 5429590c0a058bc681b450b0130ecb1f848fb740 Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Thu, 28 Dec 2023 10:56:17 +0800 Subject: [PATCH v1] Fix 'negative bitmapset member' error When removing a useless join, we'd remove PHVs that are not used at join partner rels or above the join. A PHV that references the join's relid in ph_eval_at is logically "above" the join and thus should not be removed. We have the following check for that: !bms_is_member(ojrelid, phinfo->ph_eval_at) However, in the case of SJE removing an useless inner join, 'ojrelid' is set to -1, which would trigger the "negative bitmapset member not allowed" error in bms_is_member(). We can fix it by skipping checking ojrelid for inner joins. But this patch chooses to modify bms_is_member() to return false for negative numbers instead of emitting an error. --- src/backend/nodes/bitmapset.c | 4 ++-- src/test/regress/expected/join.out | 20 ++++++++++++++++++++ src/test/regress/sql/join.sql | 8 ++++++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c index e13ecaa155..2a828ce709 100644 --- a/src/backend/nodes/bitmapset.c +++ b/src/backend/nodes/bitmapset.c @@ -477,9 +477,9 @@ bms_is_member(int x, const Bitmapset *a) int wordnum, bitnum; - /* XXX better to just return false for x<0 ? */ + /* negative number cannot be a member of the bitmapset */ if (x < 0) - elog(ERROR, "negative bitmapset member not allowed"); + return false; if (a == NULL) return false; diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 1557e17299..dc5d11317f 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -6821,6 +6821,26 @@ on true; Filter: (id IS NOT NULL) (8 rows) +-- Check that SJE removes the whole PHVs correctly +explain (verbose, costs off) +select 1 from emp1 t1 left join + (lateral (select 1 as x, * from emp1 t2) s1 inner join + (select * from emp1 t3) s2 on s1.id = s2.id) + on true +where s1.x = 1; + QUERY PLAN +--------------------------------------------------------- + Nested Loop + Output: 1 + -> Seq Scan on public.emp1 t1 + Output: t1.id, t1.code + -> Materialize + Output: t3.id + -> Seq Scan on public.emp1 t3 + Output: t3.id + Filter: ((t3.id IS NOT NULL) AND (1 = 1)) +(9 rows) + -- Check that PHVs do not impose any constraints on removing self joins explain (verbose, costs off) select * from emp1 t1 join emp1 t2 on t1.id = t2.id left join diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index fed9e83e31..e2ec5c1c12 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -2600,6 +2600,14 @@ select * from emp1 t1 left join on true) on true; +-- Check that SJE removes the whole PHVs correctly +explain (verbose, costs off) +select 1 from emp1 t1 left join + (lateral (select 1 as x, * from emp1 t2) s1 inner join + (select * from emp1 t3) s2 on s1.id = s2.id) + on true +where s1.x = 1; + -- Check that PHVs do not impose any constraints on removing self joins explain (verbose, costs off) select * from emp1 t1 join emp1 t2 on t1.id = t2.id left join -- 2.31.0