From b4fe242c46e8ec1a7a4f76a3161bb95771f08221 Mon Sep 17 00:00:00 2001 From: Rui Zhao Date: Fri, 7 Aug 2026 23:32:20 +0800 Subject: [PATCH 1/2] Keep filter-expecting paths out of pathlist A path with expected_filters set reports the rowcount it would have if a hash join above builds the filter and pushes it down, so a relation ends up with paths of the same parameterization disagreeing about their rowcount. optimizer/README forbids that, and for a reason: it is what lets add_path() discard a dominated path on cost alone and add_path_precheck() reject a candidate before it is built. v9 keeps them in pathlist anyway and teaches seven consumers to look away -- set_cheapest(), add_path(), add_path_precheck(), add_partial_path(), get_cheapest_path_for_pathkeys(), get_cheapest_fractional_path_for_pathkeys() and generate_expected_filter_paths() -- plus the six try_* sites that bypass add_path_precheck() outright, carrying an XXX asking for something cleaner. That is a rule maintained by hand across everything that reads ->pathlist, and one reader is already missing it. pathkeys.c has three "cheapest path out of a list" helpers in a row; the first two skip filter-expecting paths and get_cheapest_parallel_safe_total_inner() does not. It returns the first parallel-safe unparameterized path it finds, which is correct only because pathlist is ordered by (disabled_nodes, total_cost) -- and the discount is what moves a filter-expecting path to the front. Its four callers all want the build side of a parallel join, which never receives a pushed-down filter, so the discount cannot be earned. Logging those sites over make check gets six hits, from a tenk1 semi join in join.sql and a select from information_schema.foreign_data_wrapper_options in foreign_data.sql. PostgreSQL has met this shape before: a partial path's rowcount is per-worker, which breaks the same rule, and the answer was partial_pathlist -- a separate list with its own add_partial_path(), leaving pathlist and its invariant alone. Do the same here. Filter-expecting paths go to rel->filtered_pathlist via add_filtered_path(), which prunes within one filter set as add_partial_path() prunes within partial_pathlist. All seven guards are then deleted rather than extended, because those functions are no longer handed such a path, and pathkeys.c stops mentioning Bloom filters entirely. match_unsorted_outer() gains the new list beside pathlist, because a join of any type can carry a filter it does not realize up to the hash join that does -- v9 got that for free from pathlist, and it matters: without it the plan for one graph_table query changes to one costed at 234 against 219. Like partial_pathlist, the new field has to be cleared wherever the existing path lists are: mark_dummy_rel() and its counterpart in joinrels.c, apply_scanjoin_target_to_paths(), and create_unique_paths() and create_partial_unique_paths(), which memcpy() the whole RelOptInfo and then clear the path fields one at a time. Missing the last two lets a unique-ified rel inherit the base rel's filtered paths, and hash_inner_and_outer() then uses one that was never unique-ified as the outer of a plain inner join, which join's "semijoin inner is not seen as unique" test catches. Both readers of the new list assert path->parent == rel, which is what a missed reset looks like. add_filtered_path() prunes within one filter set using add_path()'s own comparison, required_outer axis included: a join path carrying filters can be parameterized (for instance by a relation outside the join, through the star-schema exception), and paths of different parameterizations must not displace each other here any more than in pathlist. The three try_partial_* sites lose their jfilters branches. Once filter-expecting paths are out of pathlist and partial_pathlist, neither input of a partial join can carry one: partial_pathlist never holds them, and every inner-path source used for partial joins draws from pathlist. An Assert on the inputs now says so. (v9 could reach those branches, but only through the bug fixed here: get_cheapest_parallel_safe_total_inner() handing a filtered path to a partial join.) The three try_partial_* sites lose their jfilters branches entirely: neither input of a partial join can carry an expected filter -- partial_pathlist never holds such paths, and every inner-path source used for partial joins draws from pathlist, which no longer holds them either. An Assert on the inputs replaces the dead code. (In v9 those branches are reachable, and the only way in is the unguarded get_cheapest_parallel_safe_total_inner() above.) A CustomScan provider offering a filter-expecting path must now call add_filtered_path() rather than add_path(); test_bloom_customscan is updated. --- src/backend/optimizer/README | 10 + src/backend/optimizer/path/allpaths.c | 5 +- src/backend/optimizer/path/joinpath.c | 292 ++++++++---------- src/backend/optimizer/path/joinrels.c | 1 + src/backend/optimizer/path/pathkeys.c | 30 -- src/backend/optimizer/plan/planner.c | 5 + src/backend/optimizer/util/pathnode.c | 277 ++++++++++++----- src/backend/optimizer/util/relnode.c | 4 + src/include/nodes/pathnodes.h | 5 + src/include/optimizer/pathnode.h | 1 + .../test_bloom_customscan.c | 17 +- 11 files changed, 360 insertions(+), 287 deletions(-) diff --git a/src/backend/optimizer/README b/src/backend/optimizer/README index 78a307cc52..f8a54993e6 100644 --- a/src/backend/optimizer/README +++ b/src/backend/optimizer/README @@ -1177,6 +1177,16 @@ pre-filtering of join paths in add_path_precheck. Without this rule we could never reject a parameterized path in advance of computing its rowcount estimate, which would greatly reduce the value of the pre-filter mechanism. +A path that expects a hash join above it to push a Bloom filter down to it +(see "Hash Join Bloom Filters") reports the rowcount it would have if the +filter really arrives, which does not obey that restriction. Rather than +weaken the restriction, such paths are kept out of pathlist entirely, in +filtered_pathlist, the same way partial paths are kept in partial_pathlist +because their rowcounts are per-worker. add_path(), add_path_precheck() and +set_cheapest() therefore need to know nothing about them; join path +generation takes them from their own list and add_filtered_path() prunes +within it, comparing only paths that expect the same filters. + To limit planning time, we have to avoid generating an unreasonably large number of parameterized paths. We do this by only generating parameterized relation scan paths for index scans, and then only for indexes for which diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 4bef76c086..978b12dd54 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -1480,7 +1480,7 @@ generate_expected_filter_paths(PlannerInfo *root, RelOptInfo *rel) foreach_ptr(Path, path, rel->pathlist) { /* XXX Is parameterization really a problem? Always? */ - if (path->param_info != NULL || path->expected_filters != NIL) + if (path->param_info != NULL) continue; switch (nodeTag(path)) @@ -1555,7 +1555,7 @@ generate_expected_filter_paths(PlannerInfo *root, RelOptInfo *rel) } if (newpath != NULL) - add_path(rel, newpath); + add_filtered_path(rel, newpath); } } } @@ -3139,6 +3139,7 @@ set_dummy_rel_pathlist(RelOptInfo *rel) /* Discard any pre-existing paths; no further need for them */ rel->pathlist = NIL; rel->partial_pathlist = NIL; + rel->filtered_pathlist = NIL; /* Set up the dummy path */ add_path(rel, (Path *) create_append_path(NULL, rel, in, diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c index 40f762ccc1..be9f8934f9 100644 --- a/src/backend/optimizer/path/joinpath.c +++ b/src/backend/optimizer/path/joinpath.c @@ -1267,20 +1267,11 @@ try_nestloop_path(PlannerInfo *root, } /* - * If the path expects any filters, it's excluded from the cost - * pruning performed by add_path (so don't bother with - * add_path_precheck either). Once a path has all filters satisfied - * (or there were no filters), do the pruning as usual. - * - * XXX We don't want the "regular" paths without filters to get - * removed, because we need the option to pick from join algorithms. - * Paths with filters would likely win (simply because there are fewer - * rows), but they only work with hashjoins. However, maybe the - * hashjoin won't work for some reason (e.g. it wouldn't fit into - * work_mem). - * - * XXX Maybe it'd be cleaner to do this in add_path_precheck (i.e. - * make it return true for paths with expected filters). + * A path that expects filters competes in filtered_pathlist, not + * pathlist, so add_path_precheck() -- which consults pathlist -- is + * the wrong gate for it; add_filtered_path() prunes within each + * filter set instead. Ordinary paths go through the precheck as + * usual. */ if (jfilters != NIL || add_path_precheck(joinrel, workspace.disabled_nodes, @@ -1299,8 +1290,13 @@ try_nestloop_path(PlannerInfo *root, extra->restrictlist, pathkeys, required_outer); - set_join_path_expected_filters(nlpath, jfilters); - add_path(joinrel, nlpath); + if (jfilters != NIL) + { + set_join_path_expected_filters(nlpath, jfilters); + add_filtered_path(joinrel, nlpath); + } + else + add_path(joinrel, nlpath); } else { @@ -1374,65 +1370,31 @@ try_partial_nestloop_path(PlannerInfo *root, outer_path, inner_path, extra); /* - * Account for expected Bloom filters carried by the input paths. A - * nestloop never builds a Bloom filter, so if it is the source of any - * expected filter the path is contradicted and must be rejected; - * otherwise the filters propagate to the resulting path. + * Partial join inputs never carry expected Bloom filters: paths that + * expect one live only in filtered_pathlist, which neither + * partial_pathlist nor any inner-path source used for partial joins + * draws from. */ - { - bool contradicted; - List *jfilters; + Assert(outer_path->expected_filters == NIL); + Assert(inner_path->expected_filters == NIL); - jfilters = compute_join_expected_filters(root, outer_path, inner_path, - jointype, false, NIL, - &contradicted, NULL); - - /* - * Contradicted means the inner/outer paths expect this join to - * realize one of the expected filters, but a nestloop can't do that. - * So these input paths are incompatible with a nestloop. - */ - if (contradicted) - { - return; - } - - /* - * If the path expects any filters, it's excluded from the cost - * pruning performed by add_path (so don't bother with - * add_path_precheck either). Once a path has all filters satisfied - * (or there were no filters), do the pruning as usual. - * - * XXX We don't want the "regular" paths without filters to get - * removed, because we need the option to pick from join algorithms. - * Paths with filters would likely win (simply because there are fewer - * rows), but they only work with hashjoins. However, maybe the - * hashjoin won't work for some reason (e.g. it wouldn't fit into - * work_mem). - * - * XXX Maybe it'd be cleaner to do this in add_path_precheck (i.e. - * make it return true for paths with expected filters). - */ - if (jfilters != NIL || - add_partial_path_precheck(joinrel, workspace.disabled_nodes, - workspace.startup_cost, - workspace.total_cost, pathkeys)) - { - Path *nlpath; - - nlpath = (Path *) create_nestloop_path(root, - joinrel, - jointype, - &workspace, - extra, - outer_path, - inner_path, - extra->restrictlist, - pathkeys, - NULL); - set_join_path_expected_filters(nlpath, jfilters); - add_partial_path(joinrel, nlpath); - } + if (add_partial_path_precheck(joinrel, workspace.disabled_nodes, + workspace.startup_cost, + workspace.total_cost, pathkeys)) + { + Path *nlpath; + + nlpath = (Path *) create_nestloop_path(root, + joinrel, + jointype, + &workspace, + extra, + outer_path, + inner_path, + extra->restrictlist, + pathkeys, + NULL); + add_partial_path(joinrel, nlpath); } } @@ -1568,8 +1530,13 @@ try_mergejoin_path(PlannerInfo *root, outersortkeys, innersortkeys, outer_presorted_keys); - set_join_path_expected_filters(mjpath, jfilters); - add_path(joinrel, mjpath); + if (jfilters != NIL) + { + set_join_path_expected_filters(mjpath, jfilters); + add_filtered_path(joinrel, mjpath); + } + else + add_path(joinrel, mjpath); } else { @@ -1636,51 +1603,36 @@ try_partial_mergejoin_path(PlannerInfo *root, extra); /* - * Account for expected Bloom filters carried by the input paths. A - * mergejoin never builds a Bloom filter, so it contradicts (and cannot - * use) any input path for which it would be the filter's source. - * Filter-bearing paths bypass the precheck, since their reduced cost - * isn't comparable to ordinary paths. - * - * XXX see the comments in try_nestloop_path + * Partial join inputs never carry expected Bloom filters: paths that + * expect one live only in filtered_pathlist, which neither + * partial_pathlist nor any inner-path source used for partial joins + * draws from. */ - { - bool contradicted; - List *jfilters; + Assert(outer_path->expected_filters == NIL); + Assert(inner_path->expected_filters == NIL); - jfilters = compute_join_expected_filters(root, outer_path, inner_path, - jointype, false, NIL, - &contradicted, NULL); - if (contradicted) - { - return; - } - - if (jfilters != NIL || - add_partial_path_precheck(joinrel, workspace.disabled_nodes, - workspace.startup_cost, - workspace.total_cost, pathkeys)) - { - Path *mjpath; - - /* Might be good enough to be worth trying, so let's try it. */ - mjpath = (Path *) create_mergejoin_path(root, - joinrel, - jointype, - &workspace, - extra, - outer_path, - inner_path, - extra->restrictlist, - pathkeys, - NULL, - mergeclauses, - outersortkeys, - innersortkeys, - outer_presorted_keys); - set_join_path_expected_filters(mjpath, jfilters); - add_partial_path(joinrel, mjpath); - } + if (add_partial_path_precheck(joinrel, workspace.disabled_nodes, + workspace.startup_cost, + workspace.total_cost, pathkeys)) + { + Path *mjpath; + + /* Might be good enough to be worth trying, so let's try it. */ + mjpath = (Path *) create_mergejoin_path(root, + joinrel, + jointype, + &workspace, + extra, + outer_path, + inner_path, + extra->restrictlist, + pathkeys, + NULL, + mergeclauses, + outersortkeys, + innersortkeys, + outer_presorted_keys); + add_partial_path(joinrel, mjpath); } } @@ -1782,7 +1734,10 @@ try_hashjoin_path(PlannerInfo *root, */ ((HashPath *) hjpath)->realized_filters = realized; - add_path(joinrel, hjpath); + if (jfilters != NIL) + add_filtered_path(joinrel, hjpath); + else + add_path(joinrel, hjpath); } else { @@ -1832,55 +1787,32 @@ try_partial_hashjoin_path(PlannerInfo *root, outer_path, inner_path, extra, parallel_hash); /* - * Account for expected Bloom filters carried by the input paths. A hash - * join builds and pushes down a Bloom filter, so it realizes (and removes - * from propagation) any expected filter for which it is the source; other - * filters propagate upward. Filter-bearing paths bypass the precheck. + * Partial join inputs never carry expected Bloom filters: paths that + * expect one live only in filtered_pathlist, which neither + * partial_pathlist nor any inner-path source used for partial joins + * draws from. In particular, a partial hash join never builds one. */ - { - bool contradicted; - List *jfilters; - List *realized; - - jfilters = compute_join_expected_filters(root, outer_path, inner_path, - jointype, true, hashclauses, - &contradicted, &realized); - - /* XXX Can a hashjoin contradict a filter? Probably not. */ - if (contradicted) - { - return; - } - - if (jfilters != NIL || - add_partial_path_precheck(joinrel, workspace.disabled_nodes, - workspace.startup_cost, - workspace.total_cost, NIL)) - { - Path *hjpath; - - hjpath = (Path *) create_hashjoin_path(root, - joinrel, - jointype, - &workspace, - extra, - outer_path, - inner_path, - parallel_hash, - extra->restrictlist, - NULL, - hashclauses); - set_join_path_expected_filters(hjpath, jfilters); + Assert(outer_path->expected_filters == NIL); + Assert(inner_path->expected_filters == NIL); - /* - * Record the filters this hash join realizes, so - * create_hashjoin_plan can push exactly those down (and no - * others) at plan-creation time. - */ - ((HashPath *) hjpath)->realized_filters = realized; - - add_partial_path(joinrel, hjpath); - } + if (add_partial_path_precheck(joinrel, workspace.disabled_nodes, + workspace.startup_cost, + workspace.total_cost, NIL)) + { + Path *hjpath; + + hjpath = (Path *) create_hashjoin_path(root, + joinrel, + jointype, + &workspace, + extra, + outer_path, + inner_path, + parallel_hash, + extra->restrictlist, + NULL, + hashclauses); + add_partial_path(joinrel, hjpath); } } @@ -2329,6 +2261,7 @@ match_unsorted_outer(PlannerInfo *root, { bool nestjoinOK; bool useallclauses; + List *outer_paths; Path *inner_cheapest_total = innerrel->cheapest_total_path; Path *matpath = NULL; ListCell *lc1; @@ -2409,7 +2342,26 @@ match_unsorted_outer(PlannerInfo *root, create_material_path(innerrel, inner_cheapest_total, true); } - foreach(lc1, outerrel->pathlist) + /* + * Consider the filter-bearing outer paths as well as the ordinary ones. + * A join of any type can carry a filter it does not realize up to the + * hash join that does, so these are useful here and not only under a hash + * join; they just live in their own list (see add_filtered_path). + */ + outer_paths = outerrel->pathlist; + if (outerrel->filtered_pathlist != NIL) + { + /* + * Whoever hands us this list must own it: create_unique_paths() and + * friends memcpy() a whole RelOptInfo and then clear the path lists, + * so a missed field there would surface as a path parented elsewhere. + */ + Assert(((Path *) linitial(outerrel->filtered_pathlist))->parent == outerrel); + outer_paths = list_concat_copy(outer_paths, + outerrel->filtered_pathlist); + } + + foreach(lc1, outer_paths) { Path *outerpath = (Path *) lfirst(lc1); List *merge_pathkeys; @@ -2813,12 +2765,12 @@ hash_inner_and_outer(PlannerInfo *root, * and push down the filters, so these paths are useful here even when * they would be contradicted at a non-hash join. */ - foreach(lc1, outerrel->pathlist) + foreach(lc1, outerrel->filtered_pathlist) { Path *outerpath = (Path *) lfirst(lc1); - if (outerpath->expected_filters == NIL) - continue; + /* see the matching Assert in match_unsorted_outer() */ + Assert(outerpath->parent == outerrel); if (PATH_PARAM_BY_REL(outerpath, innerrel)) continue; diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c index 70cda01ab5..b5ee21089d 100644 --- a/src/backend/optimizer/path/joinrels.c +++ b/src/backend/optimizer/path/joinrels.c @@ -2202,6 +2202,7 @@ mark_dummy_rel(RelOptInfo *rel) /* Evict any previously chosen paths */ rel->pathlist = NIL; rel->partial_pathlist = NIL; + rel->filtered_pathlist = NIL; /* Set up the dummy path */ add_path(rel, (Path *) create_append_path(NULL, rel, in, diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c index b89af1046f..5eb71635d1 100644 --- a/src/backend/optimizer/path/pathkeys.c +++ b/src/backend/optimizer/path/pathkeys.c @@ -633,21 +633,6 @@ get_cheapest_path_for_pathkeys(List *paths, List *pathkeys, if (require_parallel_safe && !path->parallel_safe) continue; - /* - * XXX We should really make this useful with pushed-down filters, - * when possible. But we have to disable that for now (by default), - * because otherwise it'd confuse merge joins - those simply get the - * cheapest sorted paths, and that's it. So we'd need to make sure to - * only consider paths that don't have conflicting filters (which the - * merge join can't satisfy). - * - * Furthermore, it'd mean the join has to consider combinations of - * inner/outer paths, while now it simply picks the cheapest ones and - * that's it. - */ - if (path->expected_filters != NIL) - continue; - /* * Since cost comparison is a lot cheaper than pathkey comparison, do * that first. (XXX is that still true?) @@ -690,21 +675,6 @@ get_cheapest_fractional_path_for_pathkeys(List *paths, { Path *path = (Path *) lfirst(l); - /* - * XXX We should really make this useful with pushed-down filters, - * when possible. But we have to disable that for now (by default), - * because otherwise it'd confuse merge joins - those simply get the - * cheapest sorted paths, and that's it. So we'd need to make sure to - * only consider paths that don't have conflicting filters (which the - * merge join can't satisfy). - * - * Furthermore, it'd mean the join has to consider combinations of - * inner/outer paths, while now it simply picks the cheapest ones and - * that's it. - */ - if (path->expected_filters != NIL) - continue; - /* * Since cost comparison is a lot cheaper than pathkey comparison, do * that first. (XXX is that still true?) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index eb853d267b..17cd9fdf4d 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -8182,7 +8182,10 @@ apply_scanjoin_target_to_paths(PlannerInfo *root, * finally zap the partial pathlist. */ if (rel_is_partitioned && IS_SIMPLE_REL(rel)) + { rel->pathlist = NIL; + rel->filtered_pathlist = NIL; + } /* * If the scan/join target is not parallel-safe, partial paths cannot @@ -8712,6 +8715,7 @@ create_unique_paths(PlannerInfo *root, RelOptInfo *rel, SpecialJoinInfo *sjinfo) unique_rel->pathlist = NIL; unique_rel->ppilist = NIL; unique_rel->partial_pathlist = NIL; + unique_rel->filtered_pathlist = NIL; unique_rel->cheapest_startup_path = NULL; unique_rel->cheapest_total_path = NULL; unique_rel->cheapest_parameterized_paths = NIL; @@ -9086,6 +9090,7 @@ create_partial_unique_paths(PlannerInfo *root, RelOptInfo *input_rel, partial_unique_rel->pathlist = NIL; partial_unique_rel->ppilist = NIL; partial_unique_rel->partial_pathlist = NIL; + partial_unique_rel->filtered_pathlist = NIL; partial_unique_rel->cheapest_startup_path = NULL; partial_unique_rel->cheapest_total_path = NULL; partial_unique_rel->cheapest_parameterized_paths = NIL; diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index ff4b2abd20..9be2b5b831 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -286,16 +286,6 @@ set_cheapest(RelOptInfo *parent_rel) Path *path = (Path *) lfirst(p); int cmp; - /* - * Paths that expect a pushed-down Bloom filter are speculative: their - * rows/cost estimates assume a hash join above will build and push a - * filter to them. They must never be chosen as the cheapest startup, - * total, or parameterized path; they are only consumed explicitly by - * join path generation (see joinpath.c). Skip them here. - */ - if (path->expected_filters != NIL) - continue; - if (path->param_info) { /* Parameterized path, so add it to parameterized_paths */ @@ -638,19 +628,6 @@ add_path(RelOptInfo *parent_rel, Path *new_path) * XXX Do it here, because all paths have to go either through add_path * or add_partial_path. But maybe there's a better place. */ -#if USE_ASSERT_CHECKING - { - ListCell *lc; - - foreach (lc, new_path->expected_filters) - { - ExpectedFilter *f = (ExpectedFilter *) lfirst(lc); - - Assert(!bms_overlap(f->build_relids, parent_rel->relids)); - } - } -#endif - /* Pretend parameterized paths have no pathkeys, per comment above */ new_path_pathkeys = new_path->param_info ? NIL : new_path->pathkeys; @@ -667,17 +644,6 @@ add_path(RelOptInfo *parent_rel, Path *new_path) PathKeysComparison keyscmp; BMS_Comparison outercmp; - /* - * Paths carrying different sets of expected Bloom filters serve - * different purposes (each may be consumed by a different parent - * join, or none at all), and their cost/row estimates aren't directly - * comparable. So if the two paths don't expect the same filters, - * keep both and don't let either dominate the other. - */ - if (!expected_filters_equal(new_path->expected_filters, - old_path->expected_filters)) - continue; - /* * Do a fuzzy cost comparison with standard fuzziness limit. */ @@ -895,20 +861,6 @@ add_path_precheck(RelOptInfo *parent_rel, int disabled_nodes, Path *old_path = (Path *) lfirst(p1); PathKeysComparison keyscmp; - /* - * Paths carrying expected Bloom filters serve a different purpose and - * are not directly cost-comparable with ordinary paths, exactly as in - * add_path (which keeps both when the expected filter sets differ). - * The candidates submitted to this precheck never carry expected - * filters of their own, so any filter-bearing old path is a - * non-comparable speculative path and must not be allowed to dominate - * (and thereby suppress) the new path. Skipping them here also - * guarantees that a join relation always retains at least one - * ordinary, filter-free path to serve as cheapest_total_path. - */ - if (old_path->expected_filters != NIL) - continue; - /* * Since the pathlist is sorted by disabled_nodes and then by * total_cost, we can stop looking once we reach a path with more @@ -1020,19 +972,6 @@ add_partial_path(RelOptInfo *parent_rel, Path *new_path) * XXX Do it here, because all paths have to go either through add_path * or add_partial_path. But maybe there's a better place. */ -#if USE_ASSERT_CHECKING - { - ListCell *lc; - - foreach (lc, new_path->expected_filters) - { - ExpectedFilter *f = (ExpectedFilter *) lfirst(lc); - - Assert(!bms_overlap(f->build_relids, parent_rel->relids)); - } - } -#endif - /* * As in add_path, throw out any paths which are dominated by the new * path, but throw out the new path if some existing path dominates it. @@ -1042,17 +981,6 @@ add_partial_path(RelOptInfo *parent_rel, Path *new_path) Path *old_path = (Path *) lfirst(p1); bool remove_old = false; /* unless new proves superior */ PathKeysComparison keyscmp; - bool filters_match; - - /* - * Paths carrying different sets of expected Bloom filters serve - * different purposes (each may be consumed by a different parent - * join, or none at all), and their cost/row estimates aren't directly - * comparable. So if the two paths don't expect the same filters, - * keep both and don't let either dominate the other. - */ - filters_match = expected_filters_equal(new_path->expected_filters, - old_path->expected_filters); /* Compare pathkeys. */ keyscmp = compare_pathkeys(new_path->pathkeys, old_path->pathkeys); @@ -1065,7 +993,7 @@ add_partial_path(RelOptInfo *parent_rel, Path *new_path) * Treat expected filters just like pathkeys - if the paths expect * different filters, they can't dominate each other. */ - if (keyscmp != PATHKEYS_DIFFERENT && filters_match) + if (keyscmp != PATHKEYS_DIFFERENT) { PathCostComparison costcmp; @@ -1112,15 +1040,10 @@ add_partial_path(RelOptInfo *parent_rel, Path *new_path) /* * new belongs after this old path if it has more disabled nodes * or if it has the same number of nodes but a greater total cost - * - * Compare the number of filters first, so that the initial path - * has no filters (there always has to be such path). */ - if ((list_length(new_path->expected_filters) > list_length(old_path->expected_filters)) || - ((list_length(new_path->expected_filters) == list_length(old_path->expected_filters)) && - (new_path->disabled_nodes > old_path->disabled_nodes || - (new_path->disabled_nodes == old_path->disabled_nodes && - new_path->total_cost >= old_path->total_cost)))) + if (new_path->disabled_nodes > old_path->disabled_nodes || + (new_path->disabled_nodes == old_path->disabled_nodes && + new_path->total_cost >= old_path->total_cost)) insert_at = foreach_current_index(p1) + 1; } @@ -1146,6 +1069,198 @@ add_partial_path(RelOptInfo *parent_rel, Path *new_path) } } +/* + * add_filtered_path + * Consider a path that expects a hash join above it to build and push a + * Bloom filter down to it, and add it to the rel's filtered_pathlist if + * it is worth keeping. + * + * Such a path is speculative: its rowcount and cost describe what happens + * only if the filters really are supplied. Two paths expecting different + * filters therefore describe different things and must not displace each + * other, and none of them describes the relation the way an ordinary path + * does. That is why they live in their own list, exactly as partial paths + * do -- the restriction in optimizer/README that all paths for a relation + * of the same parameterization share a rowcount governs pathlist, and + * keeping these out of it leaves that restriction intact, so add_path(), + * add_path_precheck() and set_cheapest() need to know nothing about them. + * + * Within one filter set we prune as add_partial_path does: by disabled + * nodes, then cost, then pathkeys. Parameterization is not considered, + * because the only consumer (hash_inner_and_outer) rejects a path + * parameterized by the other side itself. + * + * As in add_path, the caller must not modify or free the path afterwards; + * we may pfree it here. + */ +void +add_filtered_path(RelOptInfo *parent_rel, Path *new_path) +{ + bool accept_new = true; + List *new_path_pathkeys; + ListCell *p1; + + Assert(new_path->expected_filters != NIL); + + /* + * A filter whose build side is already inside this relation should have + * been realized or discarded at a lower join. + */ +#if USE_ASSERT_CHECKING + { + ListCell *lc; + + foreach(lc, new_path->expected_filters) + { + ExpectedFilter *f = (ExpectedFilter *) lfirst(lc); + + Assert(!bms_overlap(f->build_relids, parent_rel->relids)); + } + } +#endif + + /* Pretend parameterized paths have no pathkeys, as add_path does */ + new_path_pathkeys = new_path->param_info ? NIL : new_path->pathkeys; + + foreach(p1, parent_rel->filtered_pathlist) + { + Path *old_path = (Path *) lfirst(p1); + bool remove_old = false; + PathCostComparison costcmp; + PathKeysComparison keyscmp; + BMS_Comparison outercmp; + List *old_path_pathkeys; + + /* + * Paths expecting different filters are for different consumers; + * keep both. + */ + if (!expected_filters_equal(new_path->expected_filters, + old_path->expected_filters)) + continue; + + costcmp = compare_path_costs_fuzzily(new_path, old_path, + STD_FUZZ_FACTOR); + + old_path_pathkeys = old_path->param_info ? NIL : old_path->pathkeys; + keyscmp = compare_pathkeys(new_path_pathkeys, old_path_pathkeys); + + /* + * From here this is add_path's comparison verbatim, parameterization + * axis included: a join path can carry filters and still be + * parameterized, so paths of different parameterizations must not + * displace each other here any more than they do in pathlist. + */ + if (keyscmp != PATHKEYS_DIFFERENT) + { + switch (costcmp) + { + case COSTS_EQUAL: + outercmp = bms_subset_compare(PATH_REQ_OUTER(new_path), + PATH_REQ_OUTER(old_path)); + if (keyscmp == PATHKEYS_BETTER1) + { + if ((outercmp == BMS_EQUAL || + outercmp == BMS_SUBSET1) && + new_path->rows <= old_path->rows && + new_path->parallel_safe >= old_path->parallel_safe) + remove_old = true; /* new dominates old */ + } + else if (keyscmp == PATHKEYS_BETTER2) + { + if ((outercmp == BMS_EQUAL || + outercmp == BMS_SUBSET2) && + new_path->rows >= old_path->rows && + new_path->parallel_safe <= old_path->parallel_safe) + accept_new = false; /* old dominates new */ + } + else /* keyscmp == PATHKEYS_EQUAL */ + { + if (outercmp == BMS_EQUAL) + { + if (new_path->parallel_safe > + old_path->parallel_safe) + remove_old = true; /* new dominates old */ + else if (new_path->parallel_safe < + old_path->parallel_safe) + accept_new = false; /* old dominates new */ + else if (new_path->rows < old_path->rows) + remove_old = true; /* new dominates old */ + else if (new_path->rows > old_path->rows) + accept_new = false; /* old dominates new */ + else if (compare_path_costs_fuzzily(new_path, + old_path, + 1.0000000001) == COSTS_BETTER1) + remove_old = true; /* new dominates old */ + else + accept_new = false; /* old equals or + * dominates new */ + } + else if (outercmp == BMS_SUBSET1 && + new_path->rows <= old_path->rows && + new_path->parallel_safe >= old_path->parallel_safe) + remove_old = true; /* new dominates old */ + else if (outercmp == BMS_SUBSET2 && + new_path->rows >= old_path->rows && + new_path->parallel_safe <= old_path->parallel_safe) + accept_new = false; /* old dominates new */ + /* else different parameterizations, keep both */ + } + break; + case COSTS_BETTER1: + if (keyscmp != PATHKEYS_BETTER2) + { + outercmp = bms_subset_compare(PATH_REQ_OUTER(new_path), + PATH_REQ_OUTER(old_path)); + if ((outercmp == BMS_EQUAL || + outercmp == BMS_SUBSET1) && + new_path->rows <= old_path->rows && + new_path->parallel_safe >= old_path->parallel_safe) + remove_old = true; /* new dominates old */ + } + break; + case COSTS_BETTER2: + if (keyscmp != PATHKEYS_BETTER1) + { + outercmp = bms_subset_compare(PATH_REQ_OUTER(new_path), + PATH_REQ_OUTER(old_path)); + if ((outercmp == BMS_EQUAL || + outercmp == BMS_SUBSET2) && + new_path->rows >= old_path->rows && + new_path->parallel_safe <= old_path->parallel_safe) + accept_new = false; /* old dominates new */ + } + break; + case COSTS_DIFFERENT: + + /* + * can't get here, but keep this case to keep compiler + * quiet + */ + break; + } + } + + if (remove_old) + { + parent_rel->filtered_pathlist = + foreach_delete_current(parent_rel->filtered_pathlist, p1); + + /* Don't pfree IndexPaths, per the note on add_path */ + if (!IsA(old_path, IndexPath)) + pfree(old_path); + } + else if (!accept_new) + break; + } + + if (accept_new) + parent_rel->filtered_pathlist = + lappend(parent_rel->filtered_pathlist, new_path); + else if (!IsA(new_path, IndexPath)) + pfree(new_path); +} + /* * add_partial_path_precheck * Check whether a proposed new partial path could possibly get accepted. diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c index a04567b794..780e6d30ab 100644 --- a/src/backend/optimizer/util/relnode.c +++ b/src/backend/optimizer/util/relnode.c @@ -244,6 +244,7 @@ build_simple_rel(PlannerInfo *root, int relid, RelOptInfo *parent) rel->pathlist = NIL; rel->ppilist = NIL; rel->partial_pathlist = NIL; + rel->filtered_pathlist = NIL; rel->cheapest_startup_path = NULL; rel->cheapest_total_path = NULL; rel->cheapest_parameterized_paths = NIL; @@ -517,6 +518,7 @@ build_grouped_rel(PlannerInfo *root, RelOptInfo *rel) grouped_rel->pathlist = NIL; grouped_rel->ppilist = NIL; grouped_rel->partial_pathlist = NIL; + grouped_rel->filtered_pathlist = NIL; grouped_rel->cheapest_startup_path = NULL; grouped_rel->cheapest_total_path = NULL; grouped_rel->cheapest_parameterized_paths = NIL; @@ -850,6 +852,7 @@ build_join_rel(PlannerInfo *root, joinrel->pathlist = NIL; joinrel->ppilist = NIL; joinrel->partial_pathlist = NIL; + joinrel->filtered_pathlist = NIL; joinrel->cheapest_startup_path = NULL; joinrel->cheapest_total_path = NULL; joinrel->cheapest_parameterized_paths = NIL; @@ -1057,6 +1060,7 @@ build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel, joinrel->pathlist = NIL; joinrel->ppilist = NIL; joinrel->partial_pathlist = NIL; + joinrel->filtered_pathlist = NIL; joinrel->cheapest_startup_path = NULL; joinrel->cheapest_total_path = NULL; joinrel->cheapest_parameterized_paths = NIL; diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 9fc7a11313..6cfb9dff8e 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -819,6 +819,10 @@ typedef struct PartitionSchemeData *PartitionScheme; * pathlist - List of Path nodes, one for each potentially useful * method of generating the relation * ppilist - ParamPathInfo nodes for parameterized Paths, if any + * filtered_pathlist - Paths that expect a hash join above to push a + * Bloom filter down to them. Kept apart from pathlist for + * the same reason partial_pathlist is: their rowcounts do + * not obey the restriction stated below. * cheapest_startup_path - the pathlist member with lowest startup cost * (regardless of ordering) among the unparameterized paths; * or NULL if there is no unparameterized path @@ -1066,6 +1070,7 @@ typedef struct RelOptInfo List *pathlist; /* Path structures */ List *ppilist; /* ParamPathInfos used in pathlist */ List *partial_pathlist; /* partial Paths */ + List *filtered_pathlist; /* Paths expecting pushed-down filters */ struct Path *cheapest_startup_path; struct Path *cheapest_total_path; List *cheapest_parameterized_paths; diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h index 97ff59cacd..f9359f3866 100644 --- a/src/include/optimizer/pathnode.h +++ b/src/include/optimizer/pathnode.h @@ -60,6 +60,7 @@ extern bool add_path_precheck(RelOptInfo *parent_rel, int disabled_nodes, Cost startup_cost, Cost total_cost, List *pathkeys, Relids required_outer); extern void add_partial_path(RelOptInfo *parent_rel, Path *new_path); +extern void add_filtered_path(RelOptInfo *parent_rel, Path *new_path); extern bool add_partial_path_precheck(RelOptInfo *parent_rel, int disabled_nodes, Cost startup_cost, Cost total_cost, List *pathkeys); diff --git a/src/test/modules/test_bloom_customscan/test_bloom_customscan.c b/src/test/modules/test_bloom_customscan/test_bloom_customscan.c index 30f53ff45c..a1fe94bd09 100644 --- a/src/test/modules/test_bloom_customscan/test_bloom_customscan.c +++ b/src/test/modules/test_bloom_customscan/test_bloom_customscan.c @@ -98,9 +98,11 @@ static const CustomExecMethods bloom_cs_exec_methods = { * bloom_cs_add_path * Build and add one bloom-filter-capable CustomPath, optionally expecting * the given set of pushed-down filters. We don't do anything smart with - * the filters ourselves, so apply_expected_filters() supplies the same - * generic per-tuple probe cost and row-count adjustment core uses for - * stock scan types. + * the filters ourselves, so apply_expected_filters() supplies the generic + * per-tuple probe cost and row-count adjustment. Core's own scan cost + * functions take the filters as an argument instead and charge the probes + * on the tuples they fetch; a provider that costs its path itself should + * do the same and just set expected_filters. */ static void bloom_cs_add_path(RelOptInfo *rel, Cost startup_cost, Cost total_cost, @@ -127,7 +129,14 @@ bloom_cs_add_path(RelOptInfo *rel, Cost startup_cost, Cost total_cost, apply_expected_filters(&cpath->path, filters); - add_path(rel, (Path *) cpath); + /* + * A path expecting pushed-down filters belongs in filtered_pathlist, not + * pathlist -- see add_filtered_path(). + */ + if (filters != NIL) + add_filtered_path(rel, (Path *) cpath); + else + add_path(rel, (Path *) cpath); } /* -- 2.43.7