Hi Tom, Per the discussion at [1], genericcostestimate() produces estimates > that are noticeably off for small indexes, because it fails to > discount the index metapage while computing numIndexPages. > Here's a first-draft attempt at improving that. >
I reviewed this patch and it looks good to me overall. The approach is clean: each AM declares its non-leaf page count, and genericcostestimate() subtracts it from the total before the pro-rata calculation. A few observations: 1. The guard condition change from "index->pages > 1" to "index->pages > costs->numNonLeafPages" is consistent with the new formula -- it now asks "are there any leaf pages?" rather than "are there any pages beyond the first?". Arithmetic safety is also preserved: the subtraction can't go negative or zero because the guard prevents it, and divide-by-zero is still blocked by the existing "index->tuples > 1" check. 2. All AMs that use genericcostestimate() are covered: btree, hash, spgist, and bloom set numNonLeafPages = 1; GiST has no metapage so it stays at 0, which is harmless since the result doesn't change. GIN and BRIN do their own costing and are unaffected. Note that bloom lives in contrib/bloom/blcost.c, so external AMs that call genericcostestimate() may also want to set this field. 3. I agree with the decision to ignore upper btree pages -- the fanout makes them negligible, and estimating their count without catalog data would add complexity for minimal benefit. 4. The test adjustments (join.sql, memoize.sql, select.sql) all make sense as ways to preserve the original test intent despite the cost shift. However, I noticed that all test changes are defensive -- they keep existing plans from changing -- but there is no positive test case showing that the patch actually produces a better plan choice. I'm attaching a positive test case based on the motivating scenario from pgsql-performance: a tiny partial index vs a full index on the same column. Without the patch the planner picks the full index; with the patch, it correctly prefers the partial one. All regression tests pass with both patches applied. Overall, the benefit is modest but real for small indexes, and there is no downside: when numNonLeafPages is left at zero the behavior is identical to before, so existing external AM callers are unaffected as long as they zero-initialize the struct. Also, +1 for Alvaro's suggestion to change "leafs" to "leaves". Best regards, Henson
From a98e41ab58b38b4d49d24b45753032582f08b6ae Mon Sep 17 00:00:00 2001 From: Henson Choi <[email protected]> Date: Mon, 9 Mar 2026 13:42:33 +0900 Subject: [PATCH] Add test for partial vs full index selection with metapage discount --- src/test/regress/expected/select.out | 20 ++++++++++++++++++++ src/test/regress/sql/select.sql | 16 ++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/test/regress/expected/select.out b/src/test/regress/expected/select.out index 7e869b79076..0fe9ee28c35 100644 --- a/src/test/regress/expected/select.out +++ b/src/test/regress/expected/select.out @@ -908,6 +908,26 @@ select unique1, unique2 from onek2 (2 rows) RESET enable_indexscan; +-- Verify that a small partial index is preferred over a large full index +-- when both are applicable. Without discounting the metapage from index +-- page count, the partial index cost is overestimated for very small indexes. +CREATE TABLE partial_index_test (id int PRIMARY KEY, status text, + filler text DEFAULT repeat('x', 50)); +INSERT INTO partial_index_test + SELECT i, CASE WHEN i <= 5 THEN 'Canceled' ELSE 'Active' END + FROM generate_series(1, 10000) i; +CREATE INDEX pidx_status_full ON partial_index_test (status); +CREATE INDEX pidx_status_partial ON partial_index_test (status) + WHERE status = 'Canceled'; +ANALYZE partial_index_test; +explain (costs off) + SELECT status FROM partial_index_test WHERE status = 'Canceled'; + QUERY PLAN +----------------------------------------------------------------- + Index Only Scan using pidx_status_partial on partial_index_test +(1 row) + +DROP TABLE partial_index_test; -- -- Test some corner cases that have been known to confuse the planner -- diff --git a/src/test/regress/sql/select.sql b/src/test/regress/sql/select.sql index 771b9869a20..5fb7bb5c637 100644 --- a/src/test/regress/sql/select.sql +++ b/src/test/regress/sql/select.sql @@ -234,6 +234,22 @@ select unique1, unique2 from onek2 where (unique2 = 11 and stringu1 < 'B') or unique1 = 0; RESET enable_indexscan; +-- Verify that a small partial index is preferred over a large full index +-- when both are applicable. Without discounting the metapage from index +-- page count, the partial index cost is overestimated for very small indexes. +CREATE TABLE partial_index_test (id int PRIMARY KEY, status text, + filler text DEFAULT repeat('x', 50)); +INSERT INTO partial_index_test + SELECT i, CASE WHEN i <= 5 THEN 'Canceled' ELSE 'Active' END + FROM generate_series(1, 10000) i; +CREATE INDEX pidx_status_full ON partial_index_test (status); +CREATE INDEX pidx_status_partial ON partial_index_test (status) + WHERE status = 'Canceled'; +ANALYZE partial_index_test; +explain (costs off) + SELECT status FROM partial_index_test WHERE status = 'Canceled'; +DROP TABLE partial_index_test; + -- -- Test some corner cases that have been known to confuse the planner -- -- 2.43.0
