| From: | Henson Choi <assam258(at)gmail(dot)com> |
|---|---|
| To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: Accounting for metapages in genericcostestimate() |
| Date: | 2026-03-09 05:41:12 |
| Message-ID: | CAAAe_zATF2bx5tc7VZxbxFz5dZ5OO50+us48N9GN422O3qZ2XA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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
| Attachment | Content-Type | Size |
|---|---|---|
| nocfbot-0001-add-positive-test-for-metapage-discount.txt | text/plain | 3.0 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Pavel Stehule | 2026-03-09 05:53:05 | unstable regress tests modules/injection_points/syscache-update-pruned |
| Previous Message | Chao Li | 2026-03-09 05:28:32 | Re: Avoid resource leak (src/bin/pg_dump/pg_dumpall.c) |