On Tue, Sep 9, 2014 at 2:43 PM, Michael Paquier <michael.paquier@xxxxxxxxx> 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 <michael@xxxxxxxxxx> 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 (pgsql-general@xxxxxxxxxxxxxx) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-general