From d69dfaff230d923b12af03abefb329361c69fcb3 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 20 Oct 2026 07:50:51 -0400 Subject: [PATCH v1] Consider sorting the cheapest child path in generate_orderedappend_paths A fractional MergeAppend always used a child's presorted path. Also cost sorting the child's cheapest path, and take the cheaper. Bug: #19408 Reported-by: Anthony Cloutier Discussion: https://postgr.es/m/592230.1771025767@sss.pgh.pa.us --- src/backend/optimizer/path/allpaths.c | 35 +++++++++++++++++++++++---- src/test/regress/expected/inherit.out | 11 ++++++--- src/test/regress/expected/union.out | 21 ++++++++++++++++ src/test/regress/sql/inherit.sql | 2 ++ src/test/regress/sql/union.sql | 9 +++++++ 5 files changed, 70 insertions(+), 8 deletions(-) diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 72ad61e8c24..e174af80e8a 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -2030,14 +2030,39 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel, * XXX We might consider partially sorted paths too (with an * incremental sort on top). But we'd have to build all the * incremental paths, do the costing etc. - * - * Also, notice whether we actually have different paths for - * the "fractional" and "total" cases. This helps avoid - * generating two identical ordered append paths. */ if (cheapest_fractional == NULL) cheapest_fractional = cheapest_total; - else if (cheapest_fractional != cheapest_total) + + /* + * A presorted path can be far more expensive than sorting the + * cheapest-total path, for example a full index scan that + * filters out almost every row. Cost that sort (only enough + * of a Path to compare) and use it when it is at least as + * cheap, since a sort is the less risky choice. + */ + { + Path sorted_cheapest = {0}; + + cost_sort(&sorted_cheapest, root, NIL, + childrel->cheapest_total_path->disabled_nodes, + childrel->cheapest_total_path->total_cost, + childrel->cheapest_total_path->rows, + childrel->cheapest_total_path->pathtarget->width, + 0.0, work_mem, -1.0); + + if (compare_fractional_path_costs(&sorted_cheapest, + cheapest_fractional, + path_fraction) <= 0) + cheapest_fractional = childrel->cheapest_total_path; + } + + /* + * Notice whether we actually have different paths for the + * "fractional" and "total" cases. This helps avoid + * generating two identical ordered append paths. + */ + if (cheapest_fractional != cheapest_total) fraction_neq_total = true; } diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index 036fd39174f..b425a974455 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -1871,10 +1871,12 @@ insert into matest2 (name) values ('Test 4'); insert into matest3 (name) values ('Test 5'); insert into matest3 (name) values ('Test 6'); set enable_indexscan = off; -- force use of seqscan/sort, so no merge +set enable_sort = off; -- else sorted seqscans would make a merge explain (verbose, costs off) select * from matest0 order by 1-id; QUERY PLAN ------------------------------------------------------------ Sort + Disabled: true Output: matest0.id, matest0.name, ((1 - matest0.id)) Sort Key: ((1 - matest0.id)) -> Result @@ -1888,7 +1890,7 @@ explain (verbose, costs off) select * from matest0 order by 1-id; Output: matest0_3.id, matest0_3.name -> Seq Scan on public.matest3 matest0_4 Output: matest0_4.id, matest0_4.name -(14 rows) +(15 rows) select * from matest0 order by 1-id; id | name @@ -1924,6 +1926,7 @@ select min(1-id) from matest0; (1 row) reset enable_indexscan; +reset enable_sort; set enable_seqscan = off; -- plan with fewest seqscans should be merge set enable_parallel_append = off; -- Don't let parallel-append interfere explain (verbose, costs off) select * from matest0 order by 1-id; @@ -2050,7 +2053,9 @@ order by t1.b limit 10; Merge Cond: (t1.b = t2.b) -> Merge Append Sort Key: t1.b - -> Index Scan using matest0i on matest0 t1_1 + -> Sort + Sort Key: t1_1.b + -> Seq Scan on matest0 t1_1 -> Index Scan using matest1i on matest1 t1_2 -> Materialize -> Merge Append @@ -2059,7 +2064,7 @@ order by t1.b limit 10; Filter: (c = d) -> Index Scan using matest1i on matest1 t2_2 Filter: (c = d) -(14 rows) +(16 rows) reset enable_nestloop; drop table matest0 cascade; diff --git a/src/test/regress/expected/union.out b/src/test/regress/expected/union.out index 2495351d907..a92191333f5 100644 --- a/src/test/regress/expected/union.out +++ b/src/test/regress/expected/union.out @@ -1226,6 +1226,27 @@ select event_id (7 rows) drop table events_child, events, other_events; +-- Test that a presorted child path which is far more expensive than sorting +-- the child's cheapest path is not used just because it exists (bug #19408) +explain (costs off) +select * from + (select unique1 as k, unique2 as o from tenk1 + union all + select thousand as k, unique2 as o from tenk2) ss +where k = 1 order by o limit 1; + QUERY PLAN +----------------------------------------------------------- + Limit + -> Merge Append + Sort Key: tenk1.unique2 + -> Sort + Sort Key: tenk1.unique2 + -> Index Scan using tenk1_unique1 on tenk1 + Index Cond: (unique1 = 1) + -> Index Scan using tenk2_unique2 on tenk2 + Filter: (thousand = 1) +(9 rows) + reset enable_indexonlyscan; -- -- Test handling of UNION / EXCEPT / INTERSECT with provably empty inputs diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index adbbbe10b9d..7748c134178 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -751,11 +751,13 @@ insert into matest3 (name) values ('Test 5'); insert into matest3 (name) values ('Test 6'); set enable_indexscan = off; -- force use of seqscan/sort, so no merge +set enable_sort = off; -- else sorted seqscans would make a merge explain (verbose, costs off) select * from matest0 order by 1-id; select * from matest0 order by 1-id; explain (verbose, costs off) select min(1-id) from matest0; select min(1-id) from matest0; reset enable_indexscan; +reset enable_sort; set enable_seqscan = off; -- plan with fewest seqscans should be merge set enable_parallel_append = off; -- Don't let parallel-append interfere diff --git a/src/test/regress/sql/union.sql b/src/test/regress/sql/union.sql index 8fce4528a3f..b989b904dad 100644 --- a/src/test/regress/sql/union.sql +++ b/src/test/regress/sql/union.sql @@ -457,6 +457,15 @@ select event_id drop table events_child, events, other_events; +-- Test that a presorted child path which is far more expensive than sorting +-- the child's cheapest path is not used just because it exists (bug #19408) +explain (costs off) +select * from + (select unique1 as k, unique2 as o from tenk1 + union all + select thousand as k, unique2 as o from tenk2) ss +where k = 1 order by o limit 1; + reset enable_indexonlyscan; -- -- 2.37.1 (Apple Git-137.1)