Hi,
While investigating a customer-reported planner crash ("ERROR: unrecognized
node type: N"), I found a use-after-free bug in
add_path()/add_partial_path()
that turns out to be a generalization of an issue reported here in 2023,
which
stalled for lack of a self-contained reproducer:https://www.postgresql.org/message-id/CAM2%2B6%3DUC1mcVtM0Y_LEMBEGHTM58HEkqHPn7vau_V_YfuZjEGg%40mail.gmail.com This time I have a reproducer (below), and it turns out the same underlying defect shows up independently in two different places in the planner, so I think it's worth fixing generally rather than patching one call site. My teammate Shruthi internally reported the crash that got me looking at this again; once I saw it, the 3-year-old thread above came back to me, and I ended up fixing both. Thanks to her for the report. Note up front: the reproducer below (case 1) only crashes on REL_18_STABLE and earlier. On REL_19_STABLE, add_partial_path()'s dominance check was rewritten to compare startup cost as well as total cost, and that incidentally avoids the specific comparison that discards the shared path for this data. The underlying bug is still present on REL_19_STABLE; I just haven't found data that gets the new cost comparison to hit it. The problem (see follow-up email for the reproducer) ----------- add_path() and add_partial_path() pfree() a dominated old_path, or a rejected new_path, on the assumption documented in add_path()'s own header comment: "As noted in optimizer/README, deleting a previously-accepted Path is safe because we know that Paths of this rel cannot yet be referenced from any other rel, such as a higher-level join." Two existing callers violate that assumption by handing in a path that is still a live member of a *different* rel's pathlist/partial_pathlist, without copying it: 1. grouping_planner() exposes a scan/join rel's partial paths to the outer query level's final rel: foreach(lc, current_rel->partial_pathlist) { Path *partial_path = (Path *) lfirst(lc); add_partial_path(final_rel, partial_path); } without removing them from current_rel->partial_pathlist. If generate_useful_gather_paths() already built a Gather/Gather Merge path over one of those entries (it doesn't remove the entry from partial_pathlist either), and that Gather path has since been carried into final_rel->pathlist too, a later dominance comparison inside add_partial_path() can decide the shared partial path is dominated by some other promoted candidate and pfree() it -- leaving the Gather/Gather Merge path's subpath pointer dangling. This crashes planning of a UNION branch that joins parallel-safe relations and has a set-returning function in its target list. 2. create_ordered_paths() builds the ORDERED upperrel by, for each path in input_rel->pathlist that's already sorted per root->sort_pathkeys, passing that same Path pointer straight through: if (is_sorted) sorted_path = input_path; ... add_path(ordered_rel, sorted_path); input_path remains a live member of input_rel->pathlist throughout, so it's the same hazard. This is the one from the 2023 thread linked above; it's otherwise inert, since nothing else revisits the superseded input_rel's pathlist during normal planning, which is why it only ever showed up as sporadic "WARNING: could not dump unrecognized node type" while debugging postgres_fdw with an ad hoc elog(INFO, "foreignrel: %s", nodeToString(foreignrel)) dropped into postgresGetForeignPlan(). It's still reproducible today the same way, via contrib/postgres_fdw's own regression suite (the existing "subquery+MAX" test, unmodified) with that debug line temporarily restored. The fix ------- Rather than special-case each known sharing pattern, 0001 adds one general check: every Path freshly built for parent_rel already has path->parent stamped to parent_rel by its create_*_path() constructor, before add_path()/add_partial_path() is ever called with that same rel. So a path->parent != parent_rel mismatch can only mean the path is on loan from its true owning rel, and pfree()'ing it here would leave that rel's list with a dangling entry. Skip the pfree() in that case, alongside the existing IndexPath exemption (which covers a different, same-rel sharing pattern -- an IndexPath referenced as a child of a BitmapHeapPath -- that this check doesn't and needn't cover). No API changes: add_path() and add_partial_path() keep their existing signatures, and no caller needs to change. 0002 adds a regression test for the UNION+parallel+SRF crash (case 1 above) to select_parallel.sql. I did not add one for the create_ordered_paths()/postgres_fdw case, since I couldn't find a way to make that corruption observable without adding debug code. Testing ------- Developed against REL_18_STABLE (--enable-cassert, --enable-debug build): full regression suite passes, confirmed the fix addresses case 2 as well as case 1). Given this affects both add_path() and add_partial_path() and, per the 2023 thread, has been present for a while, I'd guess this warrants a back-patch once a fix is agreed on, but I'll defer to reviewers on how far back makes sense. Thanks,
v1-0001-Don-t-pfree-a-Path-that-belongs-to-a-different-re.patch
Description: Binary data
v1-0002-Add-regression-test-for-shared-partial-path-use-a.patch
Description: Binary data
