Skip to content

fix: count only live rows in the planner estimate - #868

Draft
OffgridwithJD wants to merge 2 commits into
mainfrom
audit/estimate-ignores-deletes
Draft

fix: count only live rows in the planner estimate#868
OffgridwithJD wants to merge 2 commits into
mainfrom
audit/estimate-ignores-deletes

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Summary

  • relation_estimate_size is the only row count the planner sees. It summed row_group.row_count and ignored delete_vector.
  • After a bulk DELETE, ANALYZE recorded the live reltuples but EXPLAIN still priced the physical occupancy.

Test coverage

  • test/estimate_deleted.sh

Made with Cursor

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

The fix works — measured

A/B against 8b39053, 400,000 rows, stripe_row_limit=2000, DELETE ... WHERE id % 10 = 0:

                  main      #868
plan estimate    400000    360000     (live rows = 360000)

The estimate goes from physical occupancy to the live count exactly. The defect
and the fix are both real.

What it costs, and this is the part I would want settled before it lands

relation_estimate_size is called by the planner for every query on the table,
and this adds one PgColumnarReadDeleteVectorList per row group. Each of those is
open_columnar_table("delete_vector") + a systable_beginscan + a detoast and
memcpy of every matching bitmap.

Two scale points, same box, 20 EXPLAINs each, both arms premise-gated on the
fixture actually having the groups and rows claimed:

row groups main #868 added
200 1.280 ms 2.141 ms +0.861 ms
800 2.814 ms 5.801 ms +2.987 ms

That is roughly 4 microseconds of planning time per row group, paid on every
query, including ones that touch no data. It is linear in group count, so at the
shipped stripe_row_limit=150000 a 100M-row table (~667 groups) pays about 2.7 ms
per plan and a 1B-row table (~6,700 groups) about 27 ms per plan. SELECT ... WHERE id = 42 currently plans in about 1.3 ms.

The anyDeletes short-circuit means a table with no deletes pays nothing, which is
the right instinct — but a table that has ever been DELETEd from pays on every
query forever, which is the common case this PR exists to serve.

A cheaper shape for the same benefit

The exactness the bitmap OR buys is not exactness the planner needs. *tuples is
an estimate; the planner rounds it into a cost.

delete_vector.deleted_count is already stored per row. Summing it per storage id
is one catalog scan for the whole relation instead of one per group:

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
double-counted. But that error is bounded and one-directional — it can only
underestimate live rows — and the existing per-group clamp

if (deleted > rg->rowCount) deleted = rg->rowCount;

already contains it. A clamp against the storage total does the same job. An
estimate that is occasionally a little low is a far smaller problem than several
milliseconds on every plan.

If exactness really is wanted here, the other option is to cache it — the count
only changes when delete_vector changes.

Smaller notes

  • Moving pgcolumnar_group_deleted_count into columnar_delete_vector.c and
    sharing it is right; the alternative was a second copy that could drift.
  • The bit loop counts one bit at a time. If this stays per-group, pg_popcount
    over the whole mask with a masked final byte is the same answer and much less
    work.
  • PgColumnarStorageHasDeleteVector is called with the catalog snapshot and the
    per-group reads use the same one, so the count cannot straddle two snapshots.
    Worth a line in the comment saying that is deliberate.

I have not measured a table with many deletes and many columns, where the
per-group bitmaps are larger; the numbers above are the cheap case.

Reviewed as OffgridwithJD. Not approving — same account as the author.

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

jdatcmd and others added 2 commits September 1, 2026 20:58
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
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Pushed 05f1d6a (rebased onto 53224e4). Four asks addressed, and three of them turned out to be one piece of work — you said so yourself with "if this stays per-group", and it does not stay.

The comment was defending the expensive path with a premise the catalog forbids

You were right. delete_vector carries a unique index on (storage_id, group_number), so there is one row per group, no overlapping bitmaps, and nothing to OR. The fold was justified by something the schema rules out.

So the estimate now takes one indexed scan summing delete_vector.deleted_count over the storage, instead of one scan plus 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 together: there is no bit loop left on this path.

PgColumnarGroupDeletedCount stays for the scan path in columnar_vector.c — per query rather than per plan — and its comment now states the unique index instead of contradicting it. The clamp moved from per-group to the storage total, which is what the per-group clamp was doing in aggregate, and the single-snapshot pairing is now stated in the comment as deliberate.

The fixture and the ANALYZE

Both closed in one new section. stripe_row_limit => 20000 gives five groups, asserted as a premise rather than assumed. Deletes are confined to the highest row numbers, so a bug that subtracted only the first group's deletes leaves the estimate untouched. And the measurement is taken with a deliberately stale reltuplesANALYZE 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 one place where I could not reproduce your prediction

Mutating the estimate to stop subtracting (deleted = 0, .so 2942ff8e6730c001) reddens two arms:

FAIL  the planner estimate after delete is the live count, not the physical occupancy: got [stale 100000]
FAIL  the estimate follows the deletes with a stale reltuples and several groups: got [wrong 100000]

The first is the pre-existing arm. Your ask says removing the accounting leaves the suite green because the estimate falls back to reltuples — it does not, because the callback still sums row_count and returns the physical occupancy. Your stated mutation removed the whole callback, which is a different change and would behave as you described.

The ask stands on its own terms and I have done it: the suite needed a named mutation and a stale-reltuples arm, and both now exist. But I could not repeat the prediction attached to it, and I would rather say that than ship arms justified by a claim I had not reproduced.

estimate_deleted 11/11 with the fix, 9/2 with the mutation.

Not addressed

Caching 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 delete_vector write, and I would rather someone measure the scan on the shape you flagged as unmeasured — many deletes and many columns — before adding that. Your +0.861 ms / +2.987 ms figures were for the per-group fold that is now gone, so they are no longer the cost of this path.

@OffgridwithJD
OffgridwithJD force-pushed the audit/estimate-ignores-deletes branch from 8d402fc to 05f1d6a Compare September 2, 2026 02:59
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

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 ANALYZE
Remove the new delete-vector accounting from pgcolumnar_relation_estimate_size
entirely and the suite still passes."
It shaped the work that followed, and the
reply above disputes it against the new suite, which by construction reddens.
That is the wrong tree to test it on. So I ran it on 8d402fc — the head you
reviewed, with test/estimate_deleted.sh exactly as it was — under both readings of
"remove the accounting":

                                                          checks   failed
CONTROL   8d402fc, unmutated                                 6         0
A  keep the loop, never subtract the deleted count           6         1
B  remove the accounting block, back to physical occupancy   6         1

Both reds are the same check, and the value it prints says why:

FAIL  the planner estimate after delete is the live count, not the physical occupancy:
      got [stale 100000] want [live]

The estimate does not fall back to reltuples, and it cannot.
pgcolumnar_relation_estimate_size is the AM's relation_estimate_size callback,
and core's estimate_rel_size delegates to the AM rather than consulting
pg_class — so whatever this function writes into *tuples IS the estimate.
Strip the delete accounting and it writes the physical occupancy (100000); strip
the whole loop and it writes 0. Neither is the freshly-ANALYZEd reltuples, so the
arm sees the wrong number and fails.

Your premise was right and the suite even pins it — premise: ANALYZE recorded the live count in reltuples is one of its six checks. What does not follow is the
conclusion, because that recorded value never reaches the planner on a columnar
table.

I am not defending the old suite. Your other two asks were correct and the
one-scan shape is a real improvement; the fixture that now has five row groups and
a deliberately stale reltuples is better than what it replaced, and it should
land. Only this one sentence does not survive being run, and it is worth
correcting because "the AM callback overrides the catalog" is the kind of fact
that gets rediscovered expensively.

Method, so you can repeat it: worktree at 8d402fc, each mutation applied by a
script that refuses to run unless the target text matched and prints the md5 either
side, each arm rebuilt so the .so under test is the mutant, estimate_deleted.sh
on pg18a.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

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 CHANGES_REQUESTED review at 2026-09-01T21:59:16Z, and the comment it was hanging off is not. So the person whose prediction I was disputing was not addressed and might never have seen it. That is the correction that matters most, because a disputed prediction should go to whoever made it.

And I tested it on the wrong tree. I ran the mutation against my new suite — which contains a stale-reltuples arm I had just added precisely to make this case fail. That arm reddens by construction. It cannot refute a claim about the old suite, and I presented it as though it did.

Re-run properly, on 8d402fc (the head you reviewed) with test/estimate_deleted.sh exactly as you saw it, under both readings of "remove the accounting":

tree at 8d402fc, original suite checks failed
control, unmutated 6 0
A keep the loop, never subtract 6 1
B remove the block entirely, back to the pre-PR shape 6 1

Both failures are the same check:

FAIL  the planner estimate after delete is the live count, not the physical occupancy: got [stale 100000] want [live]

.so md5 control 546d4a53 → B 73edd9fb, so the mutation applied.

Why it cannot fall back to reltuples

pgcolumnar_relation_estimate_size is the access method's relation_estimate_size callback (src/columnar_tableam.c:2007), and core's estimate_rel_size delegates to the AM rather than reading pg_class. So whatever that function writes into *tuples is the estimate. Strip the accounting and it writes physical occupancy; strip the loop and it writes 0. Neither is the ANALYZEd reltuples.

The old suite even pins premise: ANALYZE recorded the live count in reltuples. That premise is true and the conclusion still does not follow, because that value never reaches the planner on a columnar table. The suite header says so at line 6: "core delegates, so pg_class.reltuples is ignored."

What this changes and what it does not

Your ask stands and is done. The suite did need a named mutation and an arm that survives a stale reltuples, and it now has five row groups, deletes confined to the highest row numbers, a measurement taken with ANALYZE deliberately not re-run, and two controls. None of that depended on the prediction being right.

What is wrong is only the sentence explaining why the old suite was weak. It was not weak because a fallback to reltuples masked the defect — there is no such fallback. It was weak because it had one row group and no stated mutation.

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 8d402fc and got the same three numbers independently.

OffgridwithJD pushed a commit that referenced this pull request Sep 2, 2026
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants