From 669015b07e518c3b2c12c39d3d0013fd5e54f1fc Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Fri, 23 Oct 2026 21:46:53 +0900 Subject: [PATCH v3] Look through wrapper paths when collecting enforced clause serials. get_param_path_clause_serials() handles join paths and Append paths explicitly, and for anything else returns the ppi_serials of the path's ParamPathInfo on the assumption that it is a baserel scan. But Material, Memoize, Projection, Sort, IncrementalSort, Unique, Agg and GroupingSets paths all inherit their subpath's ParamPathInfo, and that assumption fails when the subpath is a join or Append. This could drop a join clause for good. If a LATERAL UNION ALL subquery references an outer relation in an expression that becomes an EquivalenceClass member, the members get no child version of it, so their parameterized scans cannot enforce the derived clause. The appendrel's ParamPathInfo claims it nonetheless, and once a Materialize sat on the Append, create_nestloop_path() dropped the clause from the join above as already enforced, returning rows that fail it. To fix, make get_param_path_clause_serials() recurse through such wrapper paths, since they enforce no clauses of their own. Also use it in get_memoize_path(), which unsafely read ppi_serials directly for its inner-unique check. To try to clarify what's going on here, introduce a new function get_wrapper_parampathinfo() that pathnode.c should use instead of directly assuming that copying a wrapper path's subpath's param_info is what to do. This is just cosmetic at present but might become less so in future. Back-patch to v16, where the serial-based detection of enforced clauses was introduced. Author: Richard Guo Co-authored-by: Tom Lane Reviewed-by: shihao zhong Reviewed-by: Matheus Alcantara Discussion: https://postgr.es/m/CAMbWs49jVFRZ7oOgMK9zYt7d=SLxpiz=dN=QMu5iV8TS+9T0AA@mail.gmail.com Backpatch-through: 16 --- src/backend/optimizer/path/joinpath.c | 6 +- src/backend/optimizer/util/pathnode.c | 20 +-- src/backend/optimizer/util/relnode.c | 187 +++++++++++++++++--------- src/include/nodes/pathnodes.h | 29 +++- src/include/optimizer/pathnode.h | 1 + src/test/regress/expected/join.out | 37 +++++ src/test/regress/sql/join.sql | 23 ++++ 7 files changed, 225 insertions(+), 78 deletions(-) diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c index dfd08e7aeb1..91bac051fcc 100644 --- a/src/backend/optimizer/path/joinpath.c +++ b/src/backend/optimizer/path/joinpath.c @@ -801,16 +801,16 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel, */ if (extra->inner_unique) { - Bitmapset *ppi_serials; + Bitmapset *pserials; if (inner_path->param_info == NULL) return NULL; - ppi_serials = inner_path->param_info->ppi_serials; + pserials = get_param_path_clause_serials(inner_path); foreach_node(RestrictInfo, rinfo, extra->restrictlist) { - if (!bms_is_member(rinfo->rinfo_serial, ppi_serials)) + if (!bms_is_member(rinfo->rinfo_serial, pserials)) return NULL; } } diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index 67c47fd7c6c..3af494f269d 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -1371,7 +1371,9 @@ create_append_path(PlannerInfo *root, * to apply get_baserel_parampathinfo to construct a full ParamPathInfo * for the path. This supports building a Memoize path atop this path, * and if this is a partitioned table the info may be useful for run-time - * pruning (cf make_partition_pruneinfo()). + * pruning (cf make_partition_pruneinfo()). Note that its ppi_clauses and + * ppi_serials may include clauses that the child paths cannot enforce; + * see comments for ParamPathInfo. * * However, if we don't have "root" then that won't work and we fall back * on the simpler get_appendrel_parampathinfo. There's no point in doing @@ -1720,7 +1722,7 @@ create_material_path(RelOptInfo *rel, Path *subpath, bool enabled) pathnode->path.pathtype = T_Material; pathnode->path.parent = rel; pathnode->path.pathtarget = rel->reltarget; - pathnode->path.param_info = subpath->param_info; + pathnode->path.param_info = get_wrapper_parampathinfo(subpath); pathnode->path.parallel_aware = false; pathnode->path.parallel_safe = rel->consider_parallel && subpath->parallel_safe; @@ -1756,7 +1758,7 @@ create_memoize_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath, pathnode->path.pathtype = T_Memoize; pathnode->path.parent = rel; pathnode->path.pathtarget = rel->reltarget; - pathnode->path.param_info = subpath->param_info; + pathnode->path.param_info = get_wrapper_parampathinfo(subpath); pathnode->path.parallel_aware = false; pathnode->path.parallel_safe = rel->consider_parallel && subpath->parallel_safe; @@ -2613,7 +2615,7 @@ create_projection_path(PlannerInfo *root, pathnode->path.pathtype = T_Result; pathnode->path.parent = rel; pathnode->path.pathtarget = target; - pathnode->path.param_info = subpath->param_info; + pathnode->path.param_info = get_wrapper_parampathinfo(subpath); pathnode->path.parallel_aware = false; pathnode->path.parallel_safe = rel->consider_parallel && subpath->parallel_safe && @@ -2868,7 +2870,7 @@ create_incremental_sort_path(PlannerInfo *root, pathnode->path.parent = rel; /* Sort doesn't project, so use source path's pathtarget */ pathnode->path.pathtarget = subpath->pathtarget; - pathnode->path.param_info = subpath->param_info; + pathnode->path.param_info = get_wrapper_parampathinfo(subpath); pathnode->path.parallel_aware = false; pathnode->path.parallel_safe = rel->consider_parallel && subpath->parallel_safe; @@ -2916,7 +2918,7 @@ create_sort_path(PlannerInfo *root, pathnode->path.parent = rel; /* Sort doesn't project, so use source path's pathtarget */ pathnode->path.pathtarget = subpath->pathtarget; - pathnode->path.param_info = subpath->param_info; + pathnode->path.param_info = get_wrapper_parampathinfo(subpath); pathnode->path.parallel_aware = false; pathnode->path.parallel_safe = rel->consider_parallel && subpath->parallel_safe; @@ -3017,7 +3019,7 @@ create_unique_path(PlannerInfo *root, pathnode->path.parent = rel; /* Unique doesn't project, so use source path's pathtarget */ pathnode->path.pathtarget = subpath->pathtarget; - pathnode->path.param_info = subpath->param_info; + pathnode->path.param_info = get_wrapper_parampathinfo(subpath); pathnode->path.parallel_aware = false; pathnode->path.parallel_safe = rel->consider_parallel && subpath->parallel_safe; @@ -3083,7 +3085,7 @@ create_agg_path(PlannerInfo *root, pathnode->path.pathtype = T_Agg; pathnode->path.parent = rel; pathnode->path.pathtarget = target; - pathnode->path.param_info = subpath->param_info; + pathnode->path.param_info = get_wrapper_parampathinfo(subpath); pathnode->path.parallel_aware = false; pathnode->path.parallel_safe = rel->consider_parallel && subpath->parallel_safe; @@ -3167,7 +3169,7 @@ create_groupingsets_path(PlannerInfo *root, pathnode->path.pathtype = T_Agg; pathnode->path.parent = rel; pathnode->path.pathtarget = target; - pathnode->path.param_info = subpath->param_info; + pathnode->path.param_info = get_wrapper_parampathinfo(subpath); pathnode->path.parallel_aware = false; pathnode->path.parallel_safe = rel->consider_parallel && subpath->parallel_safe; diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c index f2ccba52f20..f87fcf0a407 100644 --- a/src/backend/optimizer/util/relnode.c +++ b/src/backend/optimizer/util/relnode.c @@ -2152,6 +2152,27 @@ get_appendrel_parampathinfo(RelOptInfo *appendrel, Relids required_outer) return ppi; } +/* + * get_wrapper_parampathinfo + * Get the ParamPathInfo for a parameterized wrapper path. + * + * If a wrapper-type path, such as a MaterialPath, might be parameterized, + * use this function to compute its param_info. This is suitable only when + * the wrapper path cannot add or remove any parameterization from its + * subpath, which typically means that it evaluates no quals nor tlist + * expressions. + */ +ParamPathInfo * +get_wrapper_parampathinfo(Path *subpath) +{ + /* + * Currently we just re-use the subpath's ParamPathInfo, if any. The + * ppi_req_outer value is correct given the assumption stated above, and + * none of the other fields are required to be valid for a wrapper path. + */ + return subpath->param_info; +} + /* * Returns a ParamPathInfo for the parameterization given by required_outer, if * already available in the given rel. Returns NULL otherwise. @@ -2176,6 +2197,8 @@ find_param_path_info(RelOptInfo *rel, Relids required_outer) * get_param_path_clause_serials * Given a parameterized Path, return the set of pushed-down clauses * (identified by rinfo_serial numbers) enforced within the Path. + * + * Callers should use this in preference to fetching ppi_serials themselves. */ Bitmapset * get_param_path_clause_serials(Path *path) @@ -2183,72 +2206,114 @@ get_param_path_clause_serials(Path *path) if (path->param_info == NULL) return NULL; /* not parameterized */ - /* - * We don't currently support parameterized MergeAppend paths, as - * explained in the comments for generate_orderedappend_paths. - */ - Assert(!IsA(path, MergeAppendPath)); - - if (IsA(path, NestPath) || - IsA(path, MergePath) || - IsA(path, HashPath)) + switch (nodeTag(path)) { - /* - * For a join path, combine clauses enforced within either input path - * with those enforced as joinrestrictinfo in this path. Note that - * joinrestrictinfo may include some non-pushed-down clauses, but for - * current purposes it's okay if we include those in the result. (To - * be more careful, we could check for clause_relids overlapping the - * path parameterization, but it's not worth the cycles for now.) - */ - JoinPath *jpath = (JoinPath *) path; - Bitmapset *pserials; - ListCell *lc; + case T_NestPath: + case T_MergePath: + case T_HashPath: + { + /* + * For a join path, combine clauses enforced within either + * input path with those enforced as joinrestrictinfo in this + * path. Note that joinrestrictinfo may include some + * non-pushed-down clauses, but for current purposes it's okay + * if we include those in the result. (To be more careful, we + * could check for clause_relids overlapping the path + * parameterization, but it's not worth the cycles for now.) + */ + JoinPath *jpath = (JoinPath *) path; + Bitmapset *pserials; + ListCell *lc; + + pserials = NULL; + pserials = bms_add_members(pserials, + get_param_path_clause_serials(jpath->outerjoinpath)); + pserials = bms_add_members(pserials, + get_param_path_clause_serials(jpath->innerjoinpath)); + foreach(lc, jpath->joinrestrictinfo) + { + RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc); - pserials = NULL; - pserials = bms_add_members(pserials, - get_param_path_clause_serials(jpath->outerjoinpath)); - pserials = bms_add_members(pserials, - get_param_path_clause_serials(jpath->innerjoinpath)); - foreach(lc, jpath->joinrestrictinfo) - { - RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc); + pserials = bms_add_member(pserials, rinfo->rinfo_serial); + } + return pserials; + } + case T_AppendPath: + { + /* + * For an appendrel, take the intersection of the sets of + * clauses enforced in each input path. + */ + AppendPath *apath = (AppendPath *) path; + Bitmapset *pserials; + ListCell *lc; - pserials = bms_add_member(pserials, rinfo->rinfo_serial); - } - return pserials; - } - else if (IsA(path, AppendPath)) - { - /* - * For an appendrel, take the intersection of the sets of clauses - * enforced in each input path. - */ - AppendPath *apath = (AppendPath *) path; - Bitmapset *pserials; - ListCell *lc; + pserials = NULL; + foreach(lc, apath->subpaths) + { + Path *subpath = (Path *) lfirst(lc); + Bitmapset *subserials; + + subserials = get_param_path_clause_serials(subpath); + if (lc == list_head(apath->subpaths)) + pserials = bms_copy(subserials); + else + pserials = bms_int_members(pserials, subserials); + if (bms_is_empty(pserials)) + break; /* no point in continuing */ + } + return pserials; + } + case T_MergeAppendPath: - pserials = NULL; - foreach(lc, apath->subpaths) - { - Path *subpath = (Path *) lfirst(lc); - Bitmapset *subserials; + /* + * We don't currently support parameterized MergeAppend paths, as + * explained in the comments for generate_orderedappend_paths. + */ + Assert(false); + return NULL; + case T_GatherPath: + case T_GatherMergePath: - subserials = get_param_path_clause_serials(subpath); - if (lc == list_head(apath->subpaths)) - pserials = bms_copy(subserials); - else - pserials = bms_int_members(pserials, subserials); - } - return pserials; - } - else - { - /* - * Otherwise, it's a baserel path and we can use the - * previously-computed set of serial numbers. - */ - return path->param_info->ppi_serials; + /* + * Gather and GatherMerge paths are never parameterized, since we + * don't generate parameterized partial paths (see + * add_partial_path). + */ + Assert(false); + return NULL; + + /* + * A path that merely wraps another path enforces no clauses of + * its own, so recurse through such paths. We can't just use its + * ppi_serials, as that is the same as the subpath's, and won't be + * valid for join or append subpaths. The set of path types + * listed here should match the set for which pathnode.c uses + * get_wrapper_parampathinfo(). + */ + case T_MaterialPath: + return get_param_path_clause_serials(((MaterialPath *) path)->subpath); + case T_MemoizePath: + return get_param_path_clause_serials(((MemoizePath *) path)->subpath); + case T_ProjectionPath: + return get_param_path_clause_serials(((ProjectionPath *) path)->subpath); + case T_SortPath: + return get_param_path_clause_serials(((SortPath *) path)->subpath); + case T_IncrementalSortPath: + return get_param_path_clause_serials(((IncrementalSortPath *) path)->spath.subpath); + case T_UniquePath: + return get_param_path_clause_serials(((UniquePath *) path)->subpath); + case T_AggPath: + return get_param_path_clause_serials(((AggPath *) path)->subpath); + case T_GroupingSetsPath: + return get_param_path_clause_serials(((GroupingSetsPath *) path)->subpath); + default: + + /* + * Otherwise, it's a baserel path and we can use the + * previously-computed set of serial numbers. + */ + return path->param_info->ppi_serials; } } diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 8615db98a55..1140c27356a 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -1904,16 +1904,35 @@ typedef struct PathTarget * avoid recalculations, but mostly to ensure that the estimated rowcount * is in fact the same for every such path. * - * Note: ppi_clauses is only used in ParamPathInfos for base relation paths; + * ppi_req_outer, which is the lookup key for ParamPathInfo, is currently + * the only field that is guaranteed to be valid in all cases. + * + * ppi_rows is maintained for base rels and join rels, but it's not presently + * meaningful for append relations nor for wrapper paths. We copy it into + * parameterized Paths formed for base and join rels, but append paths and + * wrapper paths compute their rows estimates independently of it. + * + * ppi_clauses is the set of join clauses that could be evaluated at the + * given relation given the specified outer rels as parameter sources. + * ppi_clauses is only used in ParamPathInfos for base relation paths; * in join cases it's NIL because the set of relevant clauses varies depending * on how the join is formed. The relevant clauses will appear in each * parameterized join path's joinrestrictinfo list, instead. ParamPathInfos - * for append relations don't bother with this, either. + * for append relations likewise set ppi_clauses to NIL, mainly because + * there could be a join underneath, rendering the append's clause set just + * as variable as the join's is. The exception is that create_append_path + * uses get_baserel_parampathinfo for an appendrel that is a baserel; its + * ppi_clauses can then include clauses that no child path enforces, such as + * one derived from an EquivalenceClass member that also references another + * rel and so has no child version. * * ppi_serials is the set of rinfo_serial numbers for quals that are enforced - * by this path. As with ppi_clauses, it's only maintained for baserels. - * (We could construct it on-the-fly from ppi_clauses, but it seems better - * to materialize a copy.) + * by this path. As with ppi_clauses, it's only maintained for baserels, and + * for a baserel appendrel it can likewise claim clauses that are not really + * enforced. (We could construct it on-the-fly from ppi_clauses, but it seems + * better to materialize a copy.) Use get_param_path_clause_serials() to + * compute the set of rinfo_serial numbers for a Path that might not be for a + * baserel. */ typedef struct ParamPathInfo { diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h index da2d9b384b5..e74a95d0d22 100644 --- a/src/include/optimizer/pathnode.h +++ b/src/include/optimizer/pathnode.h @@ -376,6 +376,7 @@ extern ParamPathInfo *get_joinrel_parampathinfo(PlannerInfo *root, List **restrict_clauses); extern ParamPathInfo *get_appendrel_parampathinfo(RelOptInfo *appendrel, Relids required_outer); +extern ParamPathInfo *get_wrapper_parampathinfo(Path *subpath); extern ParamPathInfo *find_param_path_info(RelOptInfo *rel, Relids required_outer); extern Bitmapset *get_param_path_clause_serials(Path *path); diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index ca4058c8d21..1e6aa380257 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -11082,6 +11082,43 @@ select * from 4567890123456789 | -4567890123456789 | 4567890123456789 | -4567890123456789 | | | (25 rows) +-- check that a clause the UNION ALL members cannot enforce is not dropped +-- from the join above as already enforced, even through a Materialize +begin; +set local join_collapse_limit to 1; +explain (costs off) +select * from int4_tbl t1 join + (int4_tbl t2 cross join lateral + (select q1 from int8_tbl where q2 < t1.f1 + union all + select q1 from int8_tbl where q2 < t1.f1) s) + on t1.f1 = s.q1 + t1.f1; + QUERY PLAN +--------------------------------------------------------- + Nested Loop + Join Filter: (t1.f1 = (int8_tbl.q1 + t1.f1)) + -> Seq Scan on int4_tbl t1 + -> Nested Loop + -> Seq Scan on int4_tbl t2 + -> Materialize + -> Append + -> Seq Scan on int8_tbl + Filter: (q2 < t1.f1) + -> Seq Scan on int8_tbl int8_tbl_1 + Filter: (q2 < t1.f1) +(11 rows) + +select * from int4_tbl t1 join + (int4_tbl t2 cross join lateral + (select q1 from int8_tbl where q2 < t1.f1 + union all + select q1 from int8_tbl where q2 < t1.f1) s) + on t1.f1 = s.q1 + t1.f1; + f1 | f1 | q1 +----+----+---- +(0 rows) + +rollback; -- lateral can result in join conditions appearing below their -- real semantic level explain (verbose, costs off) diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 037fb7fe093..6264f23e081 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -4217,6 +4217,29 @@ select * from on z.q2 = ss.v) on y.q1 = 1; +-- check that a clause the UNION ALL members cannot enforce is not dropped +-- from the join above as already enforced, even through a Materialize +begin; + +set local join_collapse_limit to 1; + +explain (costs off) +select * from int4_tbl t1 join + (int4_tbl t2 cross join lateral + (select q1 from int8_tbl where q2 < t1.f1 + union all + select q1 from int8_tbl where q2 < t1.f1) s) + on t1.f1 = s.q1 + t1.f1; + +select * from int4_tbl t1 join + (int4_tbl t2 cross join lateral + (select q1 from int8_tbl where q2 < t1.f1 + union all + select q1 from int8_tbl where q2 < t1.f1) s) + on t1.f1 = s.q1 + t1.f1; + +rollback; + -- lateral can result in join conditions appearing below their -- real semantic level explain (verbose, costs off) -- 2.37.1 (Apple Git-137.1)