From 147b6530e892a5e7210d3ee6d8a4d8ae50c6a202 Mon Sep 17 00:00:00 2001 From: Jeevan Chalke Date: Wed, 9 Sep 2026 13:20:49 +0530 Subject: [PATCH v1 1/2] Don't pfree() a Path that belongs to a different rel than parent_rel add_path() and add_partial_path() pfree() a dominated or rejected path on the assumption, documented in add_path()'s header comment, that "Paths of this rel cannot yet be referenced from any other rel." Two existing callers violate that by passing in a path that still belongs to a different rel's pathlist/partial_pathlist: * grouping_planner() exposes a scan/join rel's partial paths to an outer query level's final rel via add_partial_path(final_rel, ...) without removing them from current_rel->partial_pathlist. If a Gather/Gather Merge already built over one of those paths has been carried into final_rel->pathlist, a later dominance comparison can pfree() the shared path, leaving that Gather's subpath dangling -- crashing planning of a UNION branch that joins parallel-safe relations with a set-returning function in the target list. Reproduced and fixed here; see the new test in select_parallel.sql. * create_ordered_paths() passes an already-sorted input_rel path straight through to add_path(ordered_rel, ...) unwrapped, hitting the same hazard. Reported to -hackers in 2023 (postgres_fdw debug output showing "unrecognized node type"), but stalled for lack of a reproducer: the corruption is otherwise inert, since nothing else revisits the superseded rel's pathlist during normal planning. CAM2+6=UC1mcVtM0Y_LEMBEGHTM58HEkqHPn7vau_V_YfuZjEGg@mail.gmail.com Fix both instances, and any future one, with a single general check instead of special-casing each: every path freshly built for parent_rel already has path->parent set to parent_rel by its constructor, so a mismatch means it's on loan from elsewhere and must not be freed here. No API changes. Verified against both repros; full regression suite passes. --- src/backend/optimizer/plan/planner.c | 8 ++++++ src/backend/optimizer/util/pathnode.c | 39 ++++++++++++++++++++++----- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 28abddea637..c4839e71b41 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -5506,6 +5506,14 @@ create_ordered_paths(PlannerInfo *root, sorted_path = apply_projection_to_path(root, ordered_rel, sorted_path, target); + /* + * When is_sorted is true, sorted_path is input_path itself -- still + * a live member of input_rel->pathlist, not a fresh path built for + * ordered_rel. add_path() detects this (sorted_path->parent is + * input_rel, not ordered_rel) and won't pfree() it even if it's + * found to be dominated here, so input_rel's own pathlist entry + * stays intact. + */ add_path(ordered_rel, sorted_path); } diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index d76f69341f2..0e83c6ec95c 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -451,9 +451,21 @@ set_cheapest(RelOptInfo *parent_rel) * from any other rel, such as a higher-level join. However, in some cases * it is possible that a Path is referenced by another Path for its own * rel; we must not delete such a Path, even if it is dominated by the new - * Path. Currently this occurs only for IndexPath objects, which may be - * referenced as children of BitmapHeapPaths as well as being paths in - * their own right. Hence, we don't pfree IndexPaths when rejecting them. + * Path. This occurs for IndexPath objects, which may be referenced as + * children of BitmapHeapPaths as well as being paths in their own right. + * Hence, we don't pfree IndexPaths when rejecting them. + * + * A related but distinct hazard is a Path that was built for, and still + * belongs to, some *other* rel than parent_rel: some callers (e.g. + * create_ordered_paths(), and grouping_planner()'s exposure of a scan/ + * join rel's partial paths to an outer query level's final rel) pass in a + * path without copying it, while it remains a live member of its true + * parent's own pathlist/partial_pathlist. We detect this generically by + * comparing the path's stamped-in path->parent against parent_rel: every + * path freshly built for parent_rel already has path->parent set to + * parent_rel by its create_*_path() constructor, so a mismatch can only + * mean the path is on loan from elsewhere and pfree'ing it here would + * leave that other rel's list with a dangling entry. * * 'parent_rel' is the relation entry to which the path corresponds. * 'new_path' is a potential path for parent_rel. @@ -632,7 +644,7 @@ add_path(RelOptInfo *parent_rel, Path *new_path) /* * Delete the data pointed-to by the deleted cell, if possible */ - if (!IsA(old_path, IndexPath)) + if (!IsA(old_path, IndexPath) && old_path->parent == parent_rel) pfree(old_path); } else @@ -665,7 +677,7 @@ add_path(RelOptInfo *parent_rel, Path *new_path) else { /* Reject and recycle the new path */ - if (!IsA(new_path, IndexPath)) + if (!IsA(new_path, IndexPath) && new_path->parent == parent_rel) pfree(new_path); } } @@ -793,6 +805,17 @@ add_path_precheck(RelOptInfo *parent_rel, int disabled_nodes, * we're done creating all partial paths for it. Unlike add_path, we don't * take an exception for IndexPaths as partial index paths won't be * referenced by partial BitmapHeapPaths. + * + * We do take an exception for a partial path that doesn't actually belong + * to parent_rel (i.e. path->parent != parent_rel). Ordinarily every + * partial path submitted here was freshly built for parent_rel, but + * grouping_planner() intentionally passes a scan/join rel's own partial + * paths to add_partial_path() for the outer query level's final rel (so + * an outer Gather can be built from them) without removing them from the + * original rel's partial_pathlist -- and if that original rel already + * built its own Gather/Gather Merge over one of them, we must not pfree + * the partial path out from under it just because it lost a comparison + * for this unrelated parent_rel. */ void add_partial_path(RelOptInfo *parent_rel, Path *new_path) @@ -879,7 +902,8 @@ add_partial_path(RelOptInfo *parent_rel, Path *new_path) { parent_rel->partial_pathlist = foreach_delete_current(parent_rel->partial_pathlist, p1); - pfree(old_path); + if (old_path->parent == parent_rel) + pfree(old_path); } else { @@ -906,7 +930,8 @@ add_partial_path(RelOptInfo *parent_rel, Path *new_path) else { /* Reject and recycle the new path */ - pfree(new_path); + if (new_path->parent == parent_rel) + pfree(new_path); } } -- 2.43.0