Here is a small patch that reproduces the problem on two tables with inheritance, and fixes it. I'll add it to the Commitfest.
On Tue, Jan 30, 2024 at 8:20 AM Ashutosh Bapat <ashutosh.bapat....@gmail.com> wrote: > > On Mon, Jan 29, 2024 at 6:11 PM Alexander Kuzmenkov > <akuzmen...@timescale.com> wrote: > > > > Hello hackers, > > > > While investigating some query plans, I noticed some code that seems > > to be wrong: when create_merge_append_path() estimates the cost of > > sorting an input, it calls cost_sort() passing subpath->parent->tuples > > as the number of tuples. Shouldn't it use subpath->parent->rows or > > even subpath->rows instead? The `tuples` variable doesn't account for > > the filters on the relation, so this leads to incorrect cost estimates > > when there are highly selective filters, and Sort + Append is chosen > > instead of MergeAppend. > > All other callers of cost_sort() except plan_cluster_use_sort() are > using rows instead of tuples. Even plan_cluster_use_sort() has > rel->rows = rel->tuples, it's actually passing rows. So agree with > your suggestion. However a test will be good since this code is quite > old. > > -- > Best Wishes, > Ashutosh Bapat
diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index 8dbf790e89..2e1ec41a54 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -1470,7 +1470,7 @@ create_merge_append_path(PlannerInfo *root, root, pathkeys, subpath->total_cost, - subpath->parent->tuples, + subpath->rows, subpath->pathtarget->width, 0.0, work_mem, diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out index 56a40d50f9..22e9ca0b10 100644 --- a/src/test/regress/expected/inherit.out +++ b/src/test/regress/expected/inherit.out @@ -1716,6 +1716,28 @@ order by t1.b limit 10; reset enable_nestloop; drop table matest0 cascade; NOTICE: drop cascades to table matest1 +-- Check that merge append is chosen in presence of filters. +create table ma0(a int primary key); +create table ma1() inherits (ma0); +insert into ma0 select generate_series(1, 10000); +insert into ma1 select generate_series(1, 10000); +analyze ma0; +analyze ma1; +explain (costs off) select * from ma0 where a < 1000 order by a; + QUERY PLAN +--------------------------------------------------- + Merge Append + Sort Key: ma0.a + -> Index Only Scan using ma0_pkey on ma0 ma0_1 + Index Cond: (a < 1000) + -> Sort + Sort Key: ma0_2.a + -> Seq Scan on ma1 ma0_2 + Filter: (a < 1000) +(8 rows) + +drop table ma0 cascade; +NOTICE: drop cascades to table ma1 -- -- Test merge-append for UNION ALL append relations -- diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql index 971aac54fd..811da462b7 100644 --- a/src/test/regress/sql/inherit.sql +++ b/src/test/regress/sql/inherit.sql @@ -624,6 +624,19 @@ reset enable_nestloop; drop table matest0 cascade; +-- Check that merge append is chosen in presence of filters. +create table ma0(a int primary key); +create table ma1() inherits (ma0); +insert into ma0 select generate_series(1, 10000); +insert into ma1 select generate_series(1, 10000); +analyze ma0; +analyze ma1; + +explain (costs off) select * from ma0 where a < 1000 order by a; + +drop table ma0 cascade; + + -- -- Test merge-append for UNION ALL append relations --
