From e38705b5c7635644a70cf7c9eadb7a90bb8ea13b Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 14 Jul 2020 18:56:49 -0400 Subject: [PATCH] Fix bitmap AND/OR scans on the inside of a nestloop partition-wise join. reparameterize_path_by_child() failed to reparameterize BitmapAnd and BitmapOr paths. This matters only if such a path is chosen as the inside of a nestloop partition-wise join, where we have to pass in parameters from the outside of the nestloop. If that did happen, we generated a bad plan that would likely lead to crashes at execution. This is not entirely reparameterize_path_by_child()'s fault though; it's the victim of an ancient decision (my ancient decision, I think) to not bother filling in param_info in BitmapAnd/Or path nodes. That caused the function to believe that such nodes and their children contain no parameter references and so need not be processed. In hindsight that decision looks pretty penny-wise and pound-foolish: while it saves a few cycles during path node setup, we do commonly need the information later. In particular, by reversing the decision and requiring valid param_info data in all nodes of a bitmap path tree, we can get rid of indxpath.c's get_bitmap_tree_required_outer() function, which computed the data on-demand. It's not unlikely that that nets out as a savings of cycles in many scenarios. A couple of other things in indxpath.c can be simplified as well. While here, get rid of some cases in reparameterize_path_by_child() that are visibly dead or useless, given that we only care about reparameterizing paths that can be on the inside of a parameterized nestloop. This case reminds one of the maxim that untested code probably does not work, so I'm unwilling to leave unreachable code in this function. (I did leave the T_Gather case in place even though it's not reached in the regression tests. It's not very clear to me when the planner might prefer to put Gather below rather than above a nestloop, but at least in principle the case might be interesting.) Per bug #16536, originally from Arne Roland but with a test case by Andrew Gierth. Back-patch to v11 where this code came in. Discussion: https://postgr.es/m/16536-2213ee0b3aad41fd@postgresql.org --- src/backend/optimizer/path/indxpath.c | 121 +++---------------- src/backend/optimizer/util/pathnode.c | 100 +++++++-------- src/test/regress/expected/partition_join.out | 104 ++++++++++++++++ src/test/regress/sql/partition_join.sql | 44 +++++++ 4 files changed, 211 insertions(+), 158 deletions(-) diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c index 2a50272da6..bcb1bc6097 100644 --- a/src/backend/optimizer/path/indxpath.c +++ b/src/backend/optimizer/path/indxpath.c @@ -122,7 +122,6 @@ static Cost bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel, List *paths); static PathClauseUsage *classify_index_clause_usage(Path *path, List **clauselist); -static Relids get_bitmap_tree_required_outer(Path *bitmapqual); static void find_indexpath_quals(Path *bitmapqual, List **quals, List **preds); static int find_list_position(Node *node, List **nodelist); static bool check_index_only(RelOptInfo *rel, IndexOptInfo *index); @@ -357,23 +356,16 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel) */ if (bitjoinpaths != NIL) { - List *path_outer; List *all_path_outers; ListCell *lc; - /* - * path_outer holds the parameterization of each path in bitjoinpaths - * (to save recalculating that several times), while all_path_outers - * holds all distinct parameterization sets. - */ - path_outer = all_path_outers = NIL; + /* Identify each distinct parameterization seen in bitjoinpaths */ + all_path_outers = NIL; foreach(lc, bitjoinpaths) { Path *path = (Path *) lfirst(lc); - Relids required_outer; + Relids required_outer = PATH_REQ_OUTER(path); - required_outer = get_bitmap_tree_required_outer(path); - path_outer = lappend(path_outer, required_outer); if (!bms_equal_any(required_outer, all_path_outers)) all_path_outers = lappend(all_path_outers, required_outer); } @@ -388,16 +380,14 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel) double loop_count; BitmapHeapPath *bpath; ListCell *lcp; - ListCell *lco; /* Identify all the bitmap join paths needing no more than that */ this_path_set = NIL; - forboth(lcp, bitjoinpaths, lco, path_outer) + foreach(lcp, bitjoinpaths) { Path *path = (Path *) lfirst(lcp); - Relids p_outers = (Relids) lfirst(lco); - if (bms_is_subset(p_outers, max_outers)) + if (bms_is_subset(PATH_REQ_OUTER(path), max_outers)) this_path_set = lappend(this_path_set, path); } @@ -411,7 +401,7 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel) bitmapqual = choose_bitmap_and(root, rel, this_path_set); /* And push that path into the mix */ - required_outer = get_bitmap_tree_required_outer(bitmapqual); + required_outer = PATH_REQ_OUTER(bitmapqual); loop_count = get_loop_count(root, rel->relid, required_outer); bpath = create_bitmap_heap_path(root, rel, bitmapqual, required_outer, loop_count, 0); @@ -1601,25 +1591,19 @@ path_usage_comparator(const void *a, const void *b) /* * Estimate the cost of actually executing a bitmap scan with a single - * index path (no BitmapAnd, at least not at this level; but it could be - * a BitmapOr). + * index path (which could be a BitmapAnd or BitmapOr node). */ static Cost bitmap_scan_cost_est(PlannerInfo *root, RelOptInfo *rel, Path *ipath) { BitmapHeapPath bpath; - Relids required_outer; - - /* Identify required outer rels, in case it's a parameterized scan */ - required_outer = get_bitmap_tree_required_outer(ipath); /* Set up a dummy BitmapHeapPath */ bpath.path.type = T_BitmapHeapPath; bpath.path.pathtype = T_BitmapHeapScan; bpath.path.parent = rel; bpath.path.pathtarget = rel->reltarget; - bpath.path.param_info = get_baserel_parampathinfo(root, rel, - required_outer); + bpath.path.param_info = ipath->param_info; bpath.path.pathkeys = NIL; bpath.bitmapqual = ipath; @@ -1628,10 +1612,13 @@ bitmap_scan_cost_est(PlannerInfo *root, RelOptInfo *rel, Path *ipath) * Parallel bitmap heap path will be considered at later stage. */ bpath.path.parallel_workers = 0; + + /* Now we can do cost_bitmap_heap_scan */ cost_bitmap_heap_scan(&bpath.path, root, rel, bpath.path.param_info, ipath, - get_loop_count(root, rel->relid, required_outer)); + get_loop_count(root, rel->relid, + PATH_REQ_OUTER(ipath))); return bpath.path.total_cost; } @@ -1643,46 +1630,15 @@ bitmap_scan_cost_est(PlannerInfo *root, RelOptInfo *rel, Path *ipath) static Cost bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel, List *paths) { - BitmapAndPath apath; - BitmapHeapPath bpath; - Relids required_outer; - - /* Set up a dummy BitmapAndPath */ - apath.path.type = T_BitmapAndPath; - apath.path.pathtype = T_BitmapAnd; - apath.path.parent = rel; - apath.path.pathtarget = rel->reltarget; - apath.path.param_info = NULL; /* not used in bitmap trees */ - apath.path.pathkeys = NIL; - apath.bitmapquals = paths; - cost_bitmap_and_node(&apath, root); - - /* Identify required outer rels, in case it's a parameterized scan */ - required_outer = get_bitmap_tree_required_outer((Path *) &apath); - - /* Set up a dummy BitmapHeapPath */ - bpath.path.type = T_BitmapHeapPath; - bpath.path.pathtype = T_BitmapHeapScan; - bpath.path.parent = rel; - bpath.path.pathtarget = rel->reltarget; - bpath.path.param_info = get_baserel_parampathinfo(root, rel, - required_outer); - bpath.path.pathkeys = NIL; - bpath.bitmapqual = (Path *) &apath; + BitmapAndPath *apath; /* - * Check the cost of temporary path without considering parallelism. - * Parallel bitmap heap path will be considered at later stage. + * Might as well build a real BitmapAndPath here, as the work is slightly + * too complicated to be worth repeating just to save one palloc. */ - bpath.path.parallel_workers = 0; + apath = create_bitmap_and_path(root, rel, paths); - /* Now we can do cost_bitmap_heap_scan */ - cost_bitmap_heap_scan(&bpath.path, root, rel, - bpath.path.param_info, - (Path *) &apath, - get_loop_count(root, rel->relid, required_outer)); - - return bpath.path.total_cost; + return bitmap_scan_cost_est(root, rel, (Path *) apath); } @@ -1753,49 +1709,6 @@ classify_index_clause_usage(Path *path, List **clauselist) } -/* - * get_bitmap_tree_required_outer - * Find the required outer rels for a bitmap tree (index/and/or) - * - * We don't associate any particular parameterization with a BitmapAnd or - * BitmapOr node; however, the IndexPaths have parameterization info, in - * their capacity as standalone access paths. The parameterization required - * for the bitmap heap scan node is the union of rels referenced in the - * child IndexPaths. - */ -static Relids -get_bitmap_tree_required_outer(Path *bitmapqual) -{ - Relids result = NULL; - ListCell *lc; - - if (IsA(bitmapqual, IndexPath)) - { - return bms_copy(PATH_REQ_OUTER(bitmapqual)); - } - else if (IsA(bitmapqual, BitmapAndPath)) - { - foreach(lc, ((BitmapAndPath *) bitmapqual)->bitmapquals) - { - result = bms_join(result, - get_bitmap_tree_required_outer((Path *) lfirst(lc))); - } - } - else if (IsA(bitmapqual, BitmapOrPath)) - { - foreach(lc, ((BitmapOrPath *) bitmapqual)->bitmapquals) - { - result = bms_join(result, - get_bitmap_tree_required_outer((Path *) lfirst(lc))); - } - } - else - elog(ERROR, "unrecognized node type: %d", nodeTag(bitmapqual)); - - return result; -} - - /* * find_indexpath_quals * diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index e845a4b1ae..5110a6b806 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -1081,11 +1081,27 @@ create_bitmap_and_path(PlannerInfo *root, List *bitmapquals) { BitmapAndPath *pathnode = makeNode(BitmapAndPath); + Relids required_outer = NULL; + ListCell *lc; pathnode->path.pathtype = T_BitmapAnd; pathnode->path.parent = rel; pathnode->path.pathtarget = rel->reltarget; - pathnode->path.param_info = NULL; /* not used in bitmap trees */ + + /* + * Identify the required outer rels as the union of what the child paths + * depend on. (Alternatively, we could insist that the caller pass this + * in, but it's more convenient and reliable to compute it here.) + */ + foreach(lc, bitmapquals) + { + Path *bitmapqual = (Path *) lfirst(lc); + + required_outer = bms_add_members(required_outer, + PATH_REQ_OUTER(bitmapqual)); + } + pathnode->path.param_info = get_baserel_parampathinfo(root, rel, + required_outer); /* * Currently, a BitmapHeapPath, BitmapAndPath, or BitmapOrPath will be @@ -1117,11 +1133,27 @@ create_bitmap_or_path(PlannerInfo *root, List *bitmapquals) { BitmapOrPath *pathnode = makeNode(BitmapOrPath); + Relids required_outer = NULL; + ListCell *lc; pathnode->path.pathtype = T_BitmapOr; pathnode->path.parent = rel; pathnode->path.pathtarget = rel->reltarget; - pathnode->path.param_info = NULL; /* not used in bitmap trees */ + + /* + * Identify the required outer rels as the union of what the child paths + * depend on. (Alternatively, we could insist that the caller pass this + * in, but it's more convenient and reliable to compute it here.) + */ + foreach(lc, bitmapquals) + { + Path *bitmapqual = (Path *) lfirst(lc); + + required_outer = bms_add_members(required_outer, + PATH_REQ_OUTER(bitmapqual)); + } + pathnode->path.param_info = get_baserel_parampathinfo(root, rel, + required_outer); /* * Currently, a BitmapHeapPath, BitmapAndPath, or BitmapOrPath will be @@ -3885,7 +3917,18 @@ do { \ !bms_overlap(PATH_REQ_OUTER(path), child_rel->top_parent_relids)) return path; - /* Reparameterize a copy of given path. */ + /* + * If possible, reparameterize the given path, making a copy. + * + * This function is currently only applied to the inner side of a nestloop + * join that is being partitioned by the partitionwise-join code. Hence, + * we need only support path types that plausibly arise in that context. + * (In particular, supporting sorted path types would be a waste of code + * and cycles: even if we translated them here, they'd just lose in + * subsequent cost comparisons.) If we do see an unsupported path type, + * that just means we won't be able to generate a partitionwise-join plan + * using that path type. + */ switch (nodeTag(path)) { case T_Path: @@ -3932,16 +3975,6 @@ do { \ } break; - case T_TidPath: - { - TidPath *tpath; - - FLAT_COPY_PATH(tpath, path, TidPath); - ADJUST_CHILD_ATTRS(tpath->tidquals); - new_path = (Path *) tpath; - } - break; - case T_ForeignPath: { ForeignPath *fpath; @@ -4032,37 +4065,6 @@ do { \ } break; - case T_MergeAppendPath: - { - MergeAppendPath *mapath; - - FLAT_COPY_PATH(mapath, path, MergeAppendPath); - REPARAMETERIZE_CHILD_PATH_LIST(mapath->subpaths); - new_path = (Path *) mapath; - } - break; - - case T_MaterialPath: - { - MaterialPath *mpath; - - FLAT_COPY_PATH(mpath, path, MaterialPath); - REPARAMETERIZE_CHILD_PATH(mpath->subpath); - new_path = (Path *) mpath; - } - break; - - case T_UniquePath: - { - UniquePath *upath; - - FLAT_COPY_PATH(upath, path, UniquePath); - REPARAMETERIZE_CHILD_PATH(upath->subpath); - ADJUST_CHILD_ATTRS(upath->uniq_exprs); - new_path = (Path *) upath; - } - break; - case T_GatherPath: { GatherPath *gpath; @@ -4073,16 +4075,6 @@ do { \ } break; - case T_GatherMergePath: - { - GatherMergePath *gmpath; - - FLAT_COPY_PATH(gmpath, path, GatherMergePath); - REPARAMETERIZE_CHILD_PATH(gmpath->subpath); - new_path = (Path *) gmpath; - } - break; - default: /* We don't know how to reparameterize this path. */ diff --git a/src/test/regress/expected/partition_join.out b/src/test/regress/expected/partition_join.out index b45a590b94..585e724375 100644 --- a/src/test/regress/expected/partition_join.out +++ b/src/test/regress/expected/partition_join.out @@ -2165,6 +2165,110 @@ SELECT t1.a, t1.c, t2.b, t2.c FROM prt1_n t1 FULL JOIN prt1 t2 ON (t1.c = t2.c); -> Seq Scan on prt1_n_p2 t1_2 (10 rows) +-- +-- Test some other plan types in a partitionwise join (unfortunately, +-- we need larger tables to get the planner to choose these plan types) +-- +create temp table prtx1 (a integer, b integer, c integer) + partition by range (a); +create temp table prtx1_1 partition of prtx1 for values from (1) to (11); +create temp table prtx1_2 partition of prtx1 for values from (11) to (21); +create temp table prtx1_3 partition of prtx1 for values from (21) to (31); +create temp table prtx2 (a integer, b integer, c integer) + partition by range (a); +create temp table prtx2_1 partition of prtx2 for values from (1) to (11); +create temp table prtx2_2 partition of prtx2 for values from (11) to (21); +create temp table prtx2_3 partition of prtx2 for values from (21) to (31); +insert into prtx1 select 1 + i%30, i, i + from generate_series(1,1000) i; +insert into prtx2 select 1 + i%30, i, i + from generate_series(1,500) i, generate_series(1,10) j; +create index on prtx2 (b); +create index on prtx2 (c); +analyze prtx1; +analyze prtx2; +explain (costs off) +select * from prtx1 +where not exists (select 1 from prtx2 + where prtx2.a=prtx1.a and prtx2.b=prtx1.b and prtx2.c=123) + and a<20 and c=120; + QUERY PLAN +------------------------------------------------------------- + Append + -> Nested Loop Anti Join + -> Seq Scan on prtx1_1 + Filter: ((a < 20) AND (c = 120)) + -> Bitmap Heap Scan on prtx2_1 + Recheck Cond: ((b = prtx1_1.b) AND (c = 123)) + Filter: (a = prtx1_1.a) + -> BitmapAnd + -> Bitmap Index Scan on prtx2_1_b_idx + Index Cond: (b = prtx1_1.b) + -> Bitmap Index Scan on prtx2_1_c_idx + Index Cond: (c = 123) + -> Nested Loop Anti Join + -> Seq Scan on prtx1_2 + Filter: ((a < 20) AND (c = 120)) + -> Bitmap Heap Scan on prtx2_2 + Recheck Cond: ((b = prtx1_2.b) AND (c = 123)) + Filter: (a = prtx1_2.a) + -> BitmapAnd + -> Bitmap Index Scan on prtx2_2_b_idx + Index Cond: (b = prtx1_2.b) + -> Bitmap Index Scan on prtx2_2_c_idx + Index Cond: (c = 123) +(23 rows) + +select * from prtx1 +where not exists (select 1 from prtx2 + where prtx2.a=prtx1.a and prtx2.b=prtx1.b and prtx2.c=123) + and a<20 and c=120; + a | b | c +---+-----+----- + 1 | 120 | 120 +(1 row) + +explain (costs off) +select * from prtx1 +where not exists (select 1 from prtx2 + where prtx2.a=prtx1.a and (prtx2.b=prtx1.b+1 or prtx2.c=99)) + and a<20 and c=91; + QUERY PLAN +----------------------------------------------------------------- + Append + -> Nested Loop Anti Join + -> Seq Scan on prtx1_1 + Filter: ((a < 20) AND (c = 91)) + -> Bitmap Heap Scan on prtx2_1 + Recheck Cond: ((b = (prtx1_1.b + 1)) OR (c = 99)) + Filter: (a = prtx1_1.a) + -> BitmapOr + -> Bitmap Index Scan on prtx2_1_b_idx + Index Cond: (b = (prtx1_1.b + 1)) + -> Bitmap Index Scan on prtx2_1_c_idx + Index Cond: (c = 99) + -> Nested Loop Anti Join + -> Seq Scan on prtx1_2 + Filter: ((a < 20) AND (c = 91)) + -> Bitmap Heap Scan on prtx2_2 + Recheck Cond: ((b = (prtx1_2.b + 1)) OR (c = 99)) + Filter: (a = prtx1_2.a) + -> BitmapOr + -> Bitmap Index Scan on prtx2_2_b_idx + Index Cond: (b = (prtx1_2.b + 1)) + -> Bitmap Index Scan on prtx2_2_c_idx + Index Cond: (c = 99) +(23 rows) + +select * from prtx1 +where not exists (select 1 from prtx2 + where prtx2.a=prtx1.a and (prtx2.b=prtx1.b+1 or prtx2.c=99)) + and a<20 and c=91; + a | b | c +---+----+---- + 2 | 91 | 91 +(1 row) + -- -- Test advanced partition-matching algorithm for partitioned join -- diff --git a/src/test/regress/sql/partition_join.sql b/src/test/regress/sql/partition_join.sql index 2a15362b1f..73606c86e5 100644 --- a/src/test/regress/sql/partition_join.sql +++ b/src/test/regress/sql/partition_join.sql @@ -463,6 +463,50 @@ SELECT t1.a, t1.c, t2.b, t2.c FROM prt1_n t1 JOIN prt2_n t2 ON (t1.c = t2.c) JOI EXPLAIN (COSTS OFF) SELECT t1.a, t1.c, t2.b, t2.c FROM prt1_n t1 FULL JOIN prt1 t2 ON (t1.c = t2.c); +-- +-- Test some other plan types in a partitionwise join (unfortunately, +-- we need larger tables to get the planner to choose these plan types) +-- +create temp table prtx1 (a integer, b integer, c integer) + partition by range (a); +create temp table prtx1_1 partition of prtx1 for values from (1) to (11); +create temp table prtx1_2 partition of prtx1 for values from (11) to (21); +create temp table prtx1_3 partition of prtx1 for values from (21) to (31); +create temp table prtx2 (a integer, b integer, c integer) + partition by range (a); +create temp table prtx2_1 partition of prtx2 for values from (1) to (11); +create temp table prtx2_2 partition of prtx2 for values from (11) to (21); +create temp table prtx2_3 partition of prtx2 for values from (21) to (31); +insert into prtx1 select 1 + i%30, i, i + from generate_series(1,1000) i; +insert into prtx2 select 1 + i%30, i, i + from generate_series(1,500) i, generate_series(1,10) j; +create index on prtx2 (b); +create index on prtx2 (c); +analyze prtx1; +analyze prtx2; + +explain (costs off) +select * from prtx1 +where not exists (select 1 from prtx2 + where prtx2.a=prtx1.a and prtx2.b=prtx1.b and prtx2.c=123) + and a<20 and c=120; + +select * from prtx1 +where not exists (select 1 from prtx2 + where prtx2.a=prtx1.a and prtx2.b=prtx1.b and prtx2.c=123) + and a<20 and c=120; + +explain (costs off) +select * from prtx1 +where not exists (select 1 from prtx2 + where prtx2.a=prtx1.a and (prtx2.b=prtx1.b+1 or prtx2.c=99)) + and a<20 and c=91; + +select * from prtx1 +where not exists (select 1 from prtx2 + where prtx2.a=prtx1.a and (prtx2.b=prtx1.b+1 or prtx2.c=99)) + and a<20 and c=91; -- -- Test advanced partition-matching algorithm for partitioned join