From 102ff824a64550c2e3c3858e75fda51e215efeaa Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 9 Oct 2026 14:56:58 -0400 Subject: [PATCH v2] 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. (v16 doesn't actually fail on the test case used here, but almost surely there are cases where it would.) Author: Richard Guo Reviewed-by: Tom Lane 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 | 16 +-- src/backend/optimizer/util/relnode.c | 177 +++++++++++++++++--------- src/include/nodes/pathnodes.h | 19 ++- src/include/optimizer/pathnode.h | 1 + src/test/regress/expected/join.out | 40 ++++++ src/test/regress/sql/join.sql | 18 +++ 7 files changed, 202 insertions(+), 75 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..4b463c17f7d 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -1720,7 +1720,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 +1756,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 +2613,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 +2868,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 +2916,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 +3017,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 +3083,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 +3167,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 34dfb757152..efb9f1aa29b 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,104 @@ 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; - 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; + /* + * 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 1c6d1fe3d04..4df2cfe057a 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -1904,16 +1904,29 @@ 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. * * 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.) + * 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 4ee94d00ade..c34948fbd91 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -10744,6 +10744,46 @@ 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 +explain (costs off) +select t1.f1 from int4_tbl t1 where t1.f1 in + (select s.v + t1.f1 from + (select q1 - 123 as v from int8_tbl where q2 < t1.f1 + union all + select q1 - 123 from int8_tbl where q2 < t1.f1) s, + int4_tbl t3) +order by 1; + QUERY PLAN +---------------------------------------------------------------- + Sort + Sort Key: t1.f1 + -> Nested Loop Semi Join + Join Filter: (t1.f1 = (((int8_tbl.q1 - 123)) + t1.f1)) + -> Seq Scan on int4_tbl t1 + -> Nested Loop + -> Seq Scan on int4_tbl t3 + -> Materialize + -> Append + -> Seq Scan on int8_tbl + Filter: (q2 < t1.f1) + -> Seq Scan on int8_tbl int8_tbl_1 + Filter: (q2 < t1.f1) +(13 rows) + +select t1.f1 from int4_tbl t1 where t1.f1 in + (select s.v + t1.f1 from + (select q1 - 123 as v from int8_tbl where q2 < t1.f1 + union all + select q1 - 123 from int8_tbl where q2 < t1.f1) s, + int4_tbl t3) +order by 1; + f1 +------------ + 123456 + 2147483647 +(2 rows) + -- 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 bf8153afbcd..50ce03394c4 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -4090,6 +4090,24 @@ 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 +explain (costs off) +select t1.f1 from int4_tbl t1 where t1.f1 in + (select s.v + t1.f1 from + (select q1 - 123 as v from int8_tbl where q2 < t1.f1 + union all + select q1 - 123 from int8_tbl where q2 < t1.f1) s, + int4_tbl t3) +order by 1; +select t1.f1 from int4_tbl t1 where t1.f1 in + (select s.v + t1.f1 from + (select q1 - 123 as v from int8_tbl where q2 < t1.f1 + union all + select q1 - 123 from int8_tbl where q2 < t1.f1) s, + int4_tbl t3) +order by 1; + -- lateral can result in join conditions appearing below their -- real semantic level explain (verbose, costs off) -- 2.52.0