On Tue, Sep 15, 2026 at 8:35 AM Richard Guo <[email protected]> wrote:
> On Wed, Sep 9, 2026 at 7:09 PM Jeevan Chalke > <[email protected]> wrote: > > 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. > > I found a query that reproduces it on v19 and master: > > set parallel_setup_cost = 0; > set parallel_tuple_cost = 0; > set min_parallel_table_scan_size = 0; > set min_parallel_index_scan_size = 0; > set max_parallel_workers_per_gather = 1; > > explain (costs off) > select unique1, generate_series(1, 5000000) from tenk1 where unique2 < 100 > union > select 1, 2; > ERROR: unrecognized node type: 3301232 > > The trick here is the SRF with a large fan-out: it adds the same large > per-row cost to both partial index scans, whose startup costs are > already equal, so their total costs end up within the fuzz factor. > When grouping_planner() re-submits them to final_rel, the sorted one > evicts the other and frees it, while the Gather built over it is still > final_rel's cheapest path. > > So this bug is real on all branches, and I think we should fix it. > > I think the parent_rel check approach in your 0001 works. The rule it > states is that add_path() and add_partial_path() free only paths built > for the rel they are building, which is what the freeing was always > relying on. > > One consequence: postgres_fdw's add_foreign_ordered_paths() and > add_foreign_final_paths() build their upper paths with the input rel > as parent, since postgresGetForeignPlan() deparses from that rel. > Under the check those paths are never freed when they lose a > comparison. That is a node or two per query, so maybe it's ok. > > The alternatives are a flat copy of Path, and reference counting (and > maybe more). Copying also fixes the bug, so maybe it would be a > workable solution too. Reference counting is more appealing in > principle, but I think it's far too large a change to back-patch. > > Thoughts? > Hi Richard, Ashutosh, Thanks for looking at this. Richard, you're right that this isn't v18-only -- I've now confirmed it reproduces the same way on REL_19_STABLE too, via the same "borrowed path" mechanism. My "only crashes on v18" line was wrong; only the exact data/costs needed to hit the fuzzy tie differ across branches. Attached, updated patches: 0001 (the fix) is unchanged; 0002 (tests) now adds your tenk1/generate_series reproducer alongside the original partitioned-UNION one -- much cheaper, no partitioning needed. On alternatives: a flat copy of the Path isn't trivial either, given how many fields (and sub-structures) some Path types carry, and reference counting is invasive as you both note. Since this needs to be backpatched, I think the path->parent check is the more practical way forward here -- a small, localized change with no API impact, versus a wider structural change on stable branches. On the postgres_fdw leak: agreed it's real, but it's bounded to a node or two reclaimed with the planning memory context, not a persistent leak. Avoiding it looks like it'd mean restructuring how postgres_fdw distinguishes the rel used for deparse from the rel that owns the pathlist, which seems like more surgery than the tradeoff warrants -- but I'll defer to your judgement on that, Richard. Richard, are you planning to take this further yourself, or should I keep pushing this patch? Just want to avoid duplicated effort. Thanks, > > - Richard > -- *Jeevan Chalke* *Senior Principal Engineer, Engineering Manager* *Product Development* enterprisedb.com <https://www.enterprisedb.com>
v2-0001-Don-t-pfree-a-Path-that-belongs-to-a-different-re.patch
Description: Binary data
v2-0002-Add-regression-test-for-shared-partial-path-use-a.patch
Description: Binary data
