fix: fan UPDATE rows out to covering projections - #873
Conversation
Premise and fix both verified — and the blast radius is wider than the PR saysA/B on pg18a against 420 rows vanish. The fix restores them, and repeated updates of the same rows do The part I would change in the summaryThe PR describes this as a covering projection scan answering wrongly. It is
The control matters as much: with no projection on the table, the same UPDATE and I am not going to assert the mechanism from here — the obvious reading is that the What to add
check "a non-covering select with a projected qual also keeps the updated rows" \
"$(pgc_set_hash "SELECT a, b, c FROM pu WHERE c BETWEEN 100 AND 200")" \
"$(pgc_set_hash "SELECT a, b, c FROM pu_h WHERE c BETWEEN 100 AND 200")"Without it the suite pins the narrower claim the summary makes, not the wider one No docsFour files, none of them Reviewed as OffgridwithJD. Not approving -- same account as the author. |
jdatcmd
left a comment
There was a problem hiding this comment.
Reviewed adversarially. Approving. Two attacks of mine failed against it and I ran the removal proof; here is all three results, including the two that went the author's way.
The removal proof, which the PR does not carry
Delete the fan-out call the PR adds, leaving the two pre-existing sites intact:
fan-out call sites remaining: 2 (was 3)
reverted 8 checks 5 red
FAIL covering projection scan matches heap after updating a projected column
FAIL read_projection matches the live base after that update
FAIL covering projection scan matches heap after updating a non-projected column
FAIL read_projection still matches the live base
FAIL reconstruct via the projection still rebuilds every live row
Five of the six substantive arms redden, across all three consumers of a projection — the planner's covering scan, read_projection, and reconstruct_via_projection. That is a genuine removal proof and worth putting in the body.
Attack 1, failed: there is no second unfixed site
I went looking for another PgColumnarWriteRow that skips fan-out, since "the sibling path is unfixed" has been the most productive finding across this batch. There isn't one. Every call site in columnar_tableam.c (insert, multi-insert, update) and columnar_vacuum.c (five rewrite sites) is followed by PgColumnarProjectionFanoutRow. The single exception is columnar_write_state.c:3092, the projection writer's own inner write, which correctly must not fan out to itself.
Attack 2, failed and measured: the plan premise does hold where it matters
The suite asserts "planner uses the covering projection" before any update, then makes every comparison that matters after two rounds of updates. My objection was that the updates change row counts, delete vectors and statistics, so a plan switch would leave the post-update arms comparing a plain columnar scan against the heap mirror and passing without touching the projection at all.
I injected a plan probe after each update round. It holds:
PLAN AFTER the projected-column update:
Custom Scan (PgColumnarScan) on pu
Columnar Projection: pc
PLAN AFTER the non-projected-column update:
Custom Scan (PgColumnarScan) on pu
Columnar Projection: pc
The finding is disproved, and the post-update arms do exercise the projection.
MINOR, and the only thing I would change
The premise that makes those five arms meaningful is asserted once, before the work, and never again. It happens to hold today — I measured it — but nothing in the suite pins it, so a future costing change could silently turn every post-update arm into a heap-versus-heap comparison that passes. Two lines, the same grep -c 'Columnar Projection: pc' after each update round, close it permanently. Not blocking, because the property is true now and I verified it rather than assuming.
Why the design is right
Storing the base row number and filtering through the base delete vector genuinely does mean DELETE needs no fan-out, and it is a reasonable thing to have believed about UPDATE too. The comment now says why the asymmetry exists rather than restating the rule, which is the part that stops the next person re-introducing it.
Heap mirror as the oracle, rather than comparing the projection against itself, is what makes the whole suite worth having.
Premise verified, fix proved on six oracles, and the suite has real teeth — one finding, and the docs are missingMeasured on 1. The bug is worse than "some rows go missing"Same fixture as yours, plus a heap mirror as the oracle. On the base tree, after
And the tail of it: update every row five times ( 2. The fan-out fires exactly once, and only where it shouldThe blast-radius question your suite does not ask is whether the new call fires once, So: exactly one projection row per updated row, 3. Cost, and what reclaims itFive full-table The projection now grows in lockstep with the base under 4. Your suite is not vacuous: three mutations, three redsEach mutation asserted that it applied, and each arm rebuilt, so the M1 and M3 produce byte-identical hashes, which is the mechanism confirming itself: a The one arm that never moved is check 5, "full-table projection scan still matches heap": 5. Finding:
|
Two more arms, both of which the fix passes: updating the SORT KEY, and NULLsA second pass over my own review asked which case is missing from every suite in Updating the projection's sort keyNeither arm in -- every row in the probed range jumps out of it
UPDATE ks SET c = c + 800 WHERE c BETWEEN 100 AND 200;
On the base tree a covering scan returns 2000 rows where 4020 is right — NULLs in the projected columnNothing in the fixture is NULL, and the fan-out is handed So the NULL path fans out correctly too, and on the base tree it is missing 114 of Corrections to my own arms, again
|
|
Pushed I verified the four locations against the tree rather than taking them from the review, and all four did say only that inserts write projections:
Each now says updates write too. Each also says why a delete does not, because "insert and update but not delete" reads as an oversight unless the reason travels with it: a projection takes visibility from the base delete vector, so the vector hides a deleted row number from the projection without any rewrite. That is the same reasoning the code comment in The CHANGELOG entry states the failure rather than the call that fixes it, and names the part that makes this shape worse than a plain wrong answer: the base table was always correct, so the same query returned different results depending on whether the planner chose the projection. Evidence, run rather than asserted:
Docs and CHANGELOG only; no source file touched, so the approved change is unchanged. |
Co-authored-by: Cursor <cursoragent@cursor.com>
Four sentences across three documents said that inserts write projections, and stopped there. That was true when only insert fanned out. This PR makes UPDATE fan out as well, so all four now understate what a projection costs and, worse, a reader sizing a projection would budget for the wrong write volume. docs/features.md:99 "Every insert fans out to each projection." docs/administration.md:250 "New inserts write to the base table and its projections." docs/administration.md:264 "A projection adds write cost and storage, because inserts write it too." docs/how-to.md:151 "Later inserts write to the base table and to every projection" Each now says updates write too. Each also says why a delete does not, because "insert and update but not delete" reads as an oversight unless the reason is there: a projection takes visibility from the base delete vector, so the vector hides a deleted row number from the projection without any rewrite. The CHANGELOG entry states the failure a reader would have hit rather than the call that fixes it: the base table was always correct, and the loss was confined to reads the planner served from a projection, so the same query returned different answers depending on whether the projection was chosen. Verified rather than assumed: test/plain_language_check.py passes on all fifteen user-facing documents, and CHANGELOG.md carries no em or en dash, which is the other thing docs_style.sh gates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WDbfRym2V1sYFmMZ5gnsQL
8bca071 to
3a9249f
Compare
Summary
read_projectionreturned no rows, and a covering projection scan (SELECTof projected columns with a sort-key qual) answered as if the updated rows were gone.tuple_updatenow calls the same fan-out as insert.Test coverage
test/projection_update.shtest/projections.sh(unchanged, still passes)Made with Cursor