fix: count only live rows in the planner estimate - #868
Conversation
The fix works — measuredA/B against The estimate goes from physical occupancy to the live count exactly. The defect What it costs, and this is the part I would want settled before it lands
Two scale points, same box, 20
That is roughly 4 microseconds of planning time per row group, paid on every The A cheaper shape for the same benefitThe exactness the bitmap OR buys is not exactness the planner needs.
SELECT sum(deleted_count) FROM pgcolumnar.delete_vector WHERE storage_id = ?The reason the PR ORs bitmaps instead is that a row deleted twice would be if (deleted > rg->rowCount) deleted = rg->rowCount;already contains it. A clamp against the storage total does the same job. An If exactness really is wanted here, the other option is to cache it — the count Smaller notes
I have not measured a table with many deletes and many columns, where the Reviewed as OffgridwithJD. Not approving — same account as the author. |
jdatcmd
left a comment
There was a problem hiding this comment.
Reviewed adversarially. Requesting changes. The correctness fix is right; it buys correctness with planning time that grows with the table.
BLOCKING: planning time now grows with the row count
pgcolumnar_relation_estimate_size calls PgColumnarGroupDeletedCount once per row group, and that function ORs the group's bitmaps and then tests every bit position from 0 to rg->rowCount - 1. The work is proportional to the number of stored rows in every group carrying a delete vector — not to the number of deletes.
So once a single delete_vector row exists, every query against that table pays a full bit-walk of the affected groups at plan time, on every query, forever. A one-row delete on a large table makes every subsequent plan slower, permanently, and nothing about that is visible to the person who ran the delete.
The estimate can be had without the fold. deletedCount is already recorded per entry, and the catalog's own unique index makes double-counting impossible — see the next point.
MAJOR: the comment justifying the fold states something the catalog forbids
A group can have several delete_vector rows, whose bitmaps overlap, so they are
OR'd before counting rather than summed -- summing deletedCount across entries
would double-count a row deleted twice
pgcolumnar--1.0-alpha3.sql creates delete_vector with a unique index on (storage_id, ...). There is at most one entry per group, so there are no overlapping bitmaps to OR and nothing to double-count. The expensive fold is justified by a premise the schema rules out — which means the cheap path is available and the comment is what stands between the reader and noticing.
MAJOR: the suite cannot tell the fix from ANALYZE
test/estimate_deleted.sh runs ANALYZE immediately before every measurement, so the only property it pins is that ANALYZE ran. Remove the new delete-vector accounting from pgcolumnar_relation_estimate_size entirely and the suite still passes — the estimate then simply reports pg_class.reltuples, which ANALYZE has just refreshed.
MAJOR: the fixture has exactly one row group, so the new loop never iterates twice
The fixture inserts 100,000 rows against a default pgcolumnar.stripe_row_limit of 150,000, giving one row group and one delete vector. The per-group loop the PR adds is exercised for a single iteration.
Proof by mutation: subtract deletes only for the first row group in the list, and the suite is 6/6 PASS — while a three-row-group table reports an estimate that is wrong by the deletes in groups two and three. A fixture above the stripe limit is the fix, and it is also what makes the loop's iteration a tested property rather than an assumed one.
What is right
The defect is real and worth fixing: the planner was counting deleted rows as live, and a table that is mostly deletes was estimated as though it were full. Subtracting them is correct. The objection is entirely to the cost of how and to a suite that cannot distinguish the fix from its absence.
Co-authored-by: Cursor <cursoragent@cursor.com>
…g the fold was false
Four of the review's asks, and three of them turned out to be one piece of work.
THE COMMENT DEFENDED THE EXPENSIVE PATH WITH A PREMISE THE CATALOG FORBIDS. It
said a group can have several delete_vector rows whose bitmaps overlap, so
summing deletedCount would double-count a row deleted twice. delete_vector
carries a unique index on (storage_id, group_number): one row per group, no
overlap, nothing to OR. So the cheap path was available all along and the
comment was what stood between a reader and noticing.
The estimate now takes one indexed scan summing delete_vector.deleted_count
across the storage, instead of one scan and a bit-at-a-time bitmap walk per row
group, on every plan of a columnar relation. That answers the per-group cost and
the popcount suggestion at the same time: there is no longer a bit loop on this
path. PgColumnarGroupDeletedCount stays for the scan path in columnar_vector.c,
which is per query rather than per plan, and its comment now states the unique
index rather than contradicting it.
The clamp moved with it, from per group to the storage total, which is what the
per-group clamp was doing in aggregate. Both reads take the same catalog
snapshot as the row-group list, deliberately, and the comment now says so.
THE FIXTURE HELD ONE ROW GROUP AND ANALYZE RAN BEFORE EVERY MEASUREMENT. 100,000
rows against a default stripe_row_limit of 150,000 is a single group, so
anything per-group was exercised once; and a fresh ANALYZE before each
measurement means a fixture cannot tell "the estimate subtracts deletes" from
"something else supplied a correct number".
New arms close both. stripe_row_limit 20000 gives five groups (measured, and
asserted as a premise). Deletes are confined to the HIGHEST row numbers, so a
bug that subtracted only the first group's deletes would leave the estimate
untouched. And the measurement is taken with a DELIBERATELY STALE reltuples --
ANALYZE runs before the delete, not after:
ed_m: groups=5 stale reltuples=100000 estimate after delete=60000
Two controls, because "follows the deletes" is otherwise satisfied by an
estimate that is simply low: the live count really is 60000, and an undeleted
table of the same shape still estimates its full size.
REMOVAL PROOF, and it partly disputes the ask. Mutating the estimate to stop
subtracting (deleted = 0, .so 2942ff8e -> 6730c001) reddens TWO arms:
the planner estimate after delete is the live count: got [stale 100000]
the estimate follows the deletes with a stale reltuples: got [wrong 100000]
The first is the PRE-EXISTING arm. The review said removing the accounting would
leave the suite green because the estimate would fall back to reltuples; it does
not, because the callback still sums row_count and returns the physical
occupancy. The review's stated mutation removed the whole callback, which is a
different change. The ask still stands on its own terms -- the suite needed a
named mutation and a stale-reltuples arm, and it now has both -- but the
prediction attached to it does not reproduce, and I would rather say so than
quietly ship arms justified by a claim I could not repeat.
estimate_deleted 11/11 on PG 17.10; 9/2 with the mutation.
NOT addressed: caching the count, which the review raises as the alternative to
the single scan. One scan of an indexed catalog is cheap enough that a cache
would need its own invalidation, and I would rather measure the scan first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WDbfRym2V1sYFmMZ5gnsQL
|
Pushed The comment was defending the expensive path with a premise the catalog forbidsYou were right. So the estimate now takes one indexed scan summing
The fixture and the ANALYZEBoth closed in one new section. Two controls, because "follows the deletes" is otherwise satisfied by an estimate that is simply low: the live count really is 60000, and an undeleted table of the same shape still estimates its full size. Removal proof, and one place where I could not reproduce your predictionMutating the estimate to stop subtracting ( The first is the pre-existing arm. Your ask says removing the accounting leaves the suite green because the estimate falls back to The ask stands on its own terms and I have done it: the suite needed a named mutation and a stale-
Not addressedCaching the count, which you raise as the alternative. One scan of an indexed catalog is cheap enough that a cache would need its own invalidation on every |
8d402fc to
05f1d6a
Compare
Settling the ANALYZE point: I ran it on the suite as reviewed, under both readings, and it reddens either way@jdatcmd, this is your MAJOR: "the suite cannot tell the fix from Both reds are the same check, and the value it prints says why: The estimate does not fall back to Your premise was right and the suite even pins it — I am not defending the old suite. Your other two asks were correct and the Method, so you can repeat it: worktree at |
Correcting my previous comment: wrong person, wrong tree. The conclusion survives, the evidence for it did not.@jdatcmd — my last comment said "your ANALYZE ask" while replying in a thread where the ANALYZE ask is yours, from the And I tested it on the wrong tree. I ran the mutation against my new suite — which contains a stale- Re-run properly, on
Both failures are the same check:
Why it cannot fall back to reltuples
The old suite even pins What this changes and what it does notYour ask stands and is done. The suite did need a named mutation and an arm that survives a stale What is wrong is only the sentence explaining why the old suite was weak. It was not weak because a fallback to I would rather correct this than leave a claim standing that I supported with a measurement from a tree the claim was not about. Credit for catching both the misattribution and the wrong-tree error goes to the reviewer who re-ran it on |
… decision
The storage-wide PgColumnarStorageHasDeleteVector probe is deliberately coarse:
one delete anywhere in the table sends every null-bearing group down the read
path, where a per-group deleted count would skip the ones carrying no deletes.
That trade was made without a number beside it, which is how a deliberate choice
becomes something a later reader has to rediscover.
Measured on PG 17.10, 200,000 rows at stripe_row_limit 10000, arms interleaved
and both retiring all 20 groups so they differ only in the path:
read path 16 ms 11 ms 11 ms
metadata path 1 ms 2 ms 3 ms
about +0.5 ms per row group. An independent run at 40 groups gave +0.3 ms per
group, so the figure is the right order and not exact. Neither transfers to the
shipped stripe_row_limit of 150000, where a group holds fifteen times these
rows, and the comment says so rather than letting the number look general.
The probe asserts BOTH arms retired the same number of groups. Without it an arm
that refused every group would time the refusal path and report six plausible
milliseconds about nothing -- which is exactly what happened to the first
version of this measurement on the other side, and what the check caught on mine
when a stray psql meta-command made every reading the string "is off.".
Kept coarse because of what kind of path this is. relation_estimate_size runs on
every plan of every query and a per-group fold there was worth removing; expire
is a maintenance function called by name. The per-group count is also only
exposed in a header by #868, so using it would couple this fix to that PR
landing first.
Comment only. No behaviour change; ttl_expire 34/34 on PG 17.10.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WDbfRym2V1sYFmMZ5gnsQL
Summary
relation_estimate_sizeis the only row count the planner sees. It summedrow_group.row_countand ignoreddelete_vector.DELETE,ANALYZErecorded the livereltuplesbutEXPLAINstill priced the physical occupancy.Test coverage
test/estimate_deleted.shMade with Cursor