On Tue, Sep 9, 2014 at 2:43 PM, Michael Paquier
<[email protected]> wrote:
> Some bisecting is showing as well that the commit at the origin of the
> regression is f343a88.
The failure is caused by an assertion not happy since this commit:
frame #4: 0x0000000101d20670
postgres`generate_bitmap_or_paths(root=0x00007fd61d004d48,
rel=0x00007fd61c033a58, clauses=0x00007fd61d010200,
other_clauses=0x0000000000000000) + 480 at indxpath.c:1213
frame #5: 0x0000000101d1fc37
postgres`create_index_paths(root=0x00007fd61d004d48,
rel=0x00007fd61c033a58) + 1255 at indxpath.c:314
frame #6: 0x0000000101d1146b
postgres`set_plain_rel_pathlist(root=0x00007fd61d004d48,
rel=0x00007fd61c033a58, rte=0x00007fd61c033c88) + 75 at allpaths.c:397
While reading the code of this commit, I noticed that
extract_or_clause has added some logic for nested OR clauses: it
extracts their content and adds them directly to the list of
subclauses that are then used by generate_bitmap_or_paths, triggering
the assertion failure reported by the trace above.
The logic for nested OR is correct by reading it, hence why not simply
removing the assertion failing? The attached patch 1 does so.
Another approach would consist in removing the nested OR part and keep
the old assertion logic, like in the patch 2 attached, but this seems
like a no-go as f343a88 has actually improved nested OR tracking.
Thoughts?
Note: I added as well a regression tests in patch 1 as this is IMO the
correct approach, if that's considered as correct of course :)
--
Michael
From 78612c5c80d57b297ef93e992874b571e9bf0f75 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Tue, 9 Sep 2014 16:43:41 +0900
Subject: [PATCH] Fix Assertion failure caused by nested OR at extraction
Commit f343a88 has added some logic in extract_or_clause to extract OR
subclauses, while the index path code in planner is wrongly assuming
that subclauses cannot be OR clauses themselves.
Per report from Benjamin Smith
---
src/backend/optimizer/path/indxpath.c | 1 -
src/test/regress/expected/join.out | 21 +++++++++++++++++++++
src/test/regress/sql/join.sql | 3 +++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index 42dcb11..88bf946 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -1210,7 +1210,6 @@ generate_bitmap_or_paths(PlannerInfo *root, RelOptInfo *rel,
List *orargs;
Assert(IsA(orarg, RestrictInfo));
- Assert(!restriction_is_or_clause((RestrictInfo *) orarg));
orargs = list_make1(orarg);
indlist = build_paths_for_OR(root, rel,
diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out
index 1cb1c51..5079ba0 100644
--- a/src/test/regress/expected/join.out
+++ b/src/test/regress/expected/join.out
@@ -2827,6 +2827,27 @@ select * from tenk1 a join tenk1 b on
Index Cond: (unique2 = 3)
(12 rows)
+explain (costs off)
+select * from tenk1 a join tenk1 b on
+ a.unique1 = 1 or ((a.unique1 = 2 or a.unique1 = 3) and b.ten = 4);
+ QUERY PLAN
+--------------------------------------------------------------------------------------------
+ Nested Loop
+ Join Filter: ((a.unique1 = 1) OR (((a.unique1 = 2) OR (a.unique1 = 3)) AND (b.ten = 4)))
+ -> Seq Scan on tenk1 b
+ -> Materialize
+ -> Bitmap Heap Scan on tenk1 a
+ Recheck Cond: ((unique1 = 1) OR ((unique1 = 2) OR (unique1 = 3)))
+ -> BitmapOr
+ -> Bitmap Index Scan on tenk1_unique1
+ Index Cond: (unique1 = 1)
+ -> BitmapOr
+ -> Bitmap Index Scan on tenk1_unique1
+ Index Cond: (unique1 = 2)
+ -> Bitmap Index Scan on tenk1_unique1
+ Index Cond: (unique1 = 3)
+(14 rows)
+
--
-- test placement of movable quals in a parameterized join tree
--
diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql
index fa3e068..c170b09 100644
--- a/src/test/regress/sql/join.sql
+++ b/src/test/regress/sql/join.sql
@@ -774,6 +774,9 @@ select * from tenk1 a join tenk1 b on
explain (costs off)
select * from tenk1 a join tenk1 b on
(a.unique1 = 1 and b.unique1 = 2) or (a.unique2 = 3 and b.ten = 4);
+explain (costs off)
+select * from tenk1 a join tenk1 b on
+ a.unique1 = 1 or ((a.unique1 = 2 or a.unique1 = 3) and b.ten = 4);
--
-- test placement of movable quals in a parameterized join tree
--
2.1.0
diff --git a/src/backend/optimizer/util/orclauses.c b/src/backend/optimizer/util/orclauses.c
index 9e954d0..387a308 100644
--- a/src/backend/optimizer/util/orclauses.c
+++ b/src/backend/optimizer/util/orclauses.c
@@ -190,21 +190,8 @@ extract_or_clause(RestrictInfo *or_rinfo, RelOptInfo *rel)
RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc2);
Assert(IsA(rinfo, RestrictInfo));
- if (restriction_is_or_clause(rinfo))
- {
- /*
- * Recurse to deal with nested OR. Note we *must* recurse
- * here, this isn't just overly-tense optimization: we
- * have to descend far enough to find and strip all
- * RestrictInfos in the expression.
- */
- Expr *suborclause;
-
- suborclause = extract_or_clause(rinfo, rel);
- if (suborclause)
- subclauses = lappend(subclauses, suborclause);
- }
- else if (is_safe_restriction_clause_for(rinfo, rel))
+ if (!restriction_is_or_clause(rinfo) &&
+ is_safe_restriction_clause_for(rinfo, rel))
subclauses = lappend(subclauses, rinfo->clause);
}
}
--
Sent via pgsql-general mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-general
