fix: vouch for no index entry we cannot prove dead to everyone (#838) - #839
Conversation
SELECT count(*) returned 228 where the truth was 400, at shipped defaults, on a table whose rows were all live. count(b) on the same table in the same session returned 400. The first got an Index Only Scan, the second the columnar scan, and the index had been emptied of entries for live rows. table_index_delete_tuples asks whether an index entry is dead to EVERYONE. heapam answers from a global visibility test, InitNonVacuumableSnapshot over GlobalVisTestFor. This answered from the calling backend's MVCC snapshot, which is a different question: a row the current, still-uncommitted transaction has deleted reads as not live while remaining live to every other session, and PgColumnarRowIsLive consults this backend's unflushed delete buffer as well. nbtree then removes those items in _bt_delitems_delete, physically, inside a critical section and WAL-logged. Nothing restores them, and ROLLBACK does not. Native storage carries no per-row xid to compare against a global horizon. The delete vector is a bitmap and its visibility comes from the MVCC catalog rows that hold it, so this callback cannot prove global deadness. It now vouches for nothing instead of guessing. For a bottom-up caller that costs nothing in correctness terms: the work is speculative by contract and _bt_delitems_delete_check returns cleanly on an empty deltids array, where its assertion is Assert(delstate->bottomup). The price is that version churn no longer reclaims leaf space on a columnar table, so such an index grows where it used to be compacted. That is space, not answers. A simple-deletion caller has already marked its entries from LP_DEAD hints and is not asking this callback to discover anything, so its marks are left exactly as they arrived. The new suite drives the shape and carries the two controls that make it specific. Churn on the INDEXED column makes index_unchanged_by_update() false so no bottom-up pass runs, and a heap with a second index takes the same indexUnchanged hint. Both must stay complete, and do. It also asserts the plan it measures is an Index Scan, so a green arm cannot come from the index never being consulted. Removal proof, against the original implementation rather than a guard deletion. The first attempt deleted only the new guard, which leaves a third behaviour that neither marks entries nor compacts the array; that tripped Assert(ndeletable > 0 || nupdatable > 0) and put the cluster into recovery, which is a red for the wrong reason and proves nothing about row loss. Restoring the original snapshot-based loop gives the right red: arm A 0 of 400, arm D 228 of 400, and count(*) 228 against count(b) 400 at defaults, with both controls still clean. The installed .so differed, ece7896a10fc unfixed against f425a69f6b87. native_index, native_delete_vector_index, native_index_projection, index_only, native_ios, native_delete_visibility_paths, native_dml, native_fetch_cache, index_fetch_penalty_width, native_index_fetch_stripe_cost and isolation all pass unchanged, as do unique_conc, update_conc and concurrency.
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Verified by running, and the load-bearing claim checked against PostgreSQL's own
source on every major we support. Three arms at 5df45b1e, base 808cd46.
The fix is pinned by the test
| arm | result |
|---|---|
A parent src/ + this PR's test/index_delete_liveness.sh |
RED |
| B head as submitted | GREEN |
C head with src/ reverted to 808cd46 |
RED |
Arm C's src/ is byte-identical to the parent under git diff --quiet.
The nbtree contract, checked rather than taken
Setting delstate->ndeltids = 0 is only safe if nbtree tolerates an empty array, so I
read _bt_delitems_delete_check in the actual sources rather than trusting the
comment. It is better than tolerated — it is documented:
/*
* Note that deltids array might be a lot smaller now. It might even have
* no entries at all (with bottom-up deletion caller), in which case there
* is nothing left to do.
*/
qsort(delstate->deltids, delstate->ndeltids, sizeof(TM_IndexDelete), _bt_delitems_cmp);
if (delstate->ndeltids == 0)
{
Assert(delstate->bottomup);
return;
}Identical in 15.18, 16.14, 17.6, 18.4 and 19beta2 — checked in each tree, not
extrapolated from one. So the decline is safe across the whole support range.
That block also proves your if (delstate->bottomup) guard is necessary, not
merely tidy: zeroing on the simple-deletion path would trip that Assert. Your comment
says "wrong twice over" and the Assert is the second of the two.
The cost is bounded, and the documentation already described the outcome
You call the cost "an index grows where it used to be compacted". Two things sharpen
that, both in your favour:
It is bounded. columnar_vacuum.c:1373 and :1521 call
PgColumnarReindexRelation, so the compaction path rebuilds the indexes. The
reclamation moves from opportunistic to explicit rather than disappearing.
docs/limitations.md:596-597 already says exactly this:
Stale index entries left by deletes and updates are filtered on fetch and reclaimed
byREINDEX, not removed opportunistically.
That sentence was already true of the intended design and false of the code. This PR
makes the code match a document that was right all along, which is a stronger argument
for declining than the one the commit makes — and it is why I do not think this PR
owes a documentation change despite touching user-visible behaviour.
The suite is built the way this kind of claim needs
Two controls, and they are what make the arm specific rather than suggestive:
indexed-column churn (a = a) makes index_unchanged_by_update() false so no
bottom-up pass runs, and a heap with a second index takes the same hint. Both must
lose nothing, and a fault that broke them both would be a different fault.
The plan premise is the part I would have asked for if it were missing: arm A really plans an Index Scan, so the arm measures the index. Without it a fallback to a
sequential scan would answer correctly and the arm would pass while measuring nothing.
The ROLLBACK arm is a good choice too — 0 of 400 reachable is a far louder signal
than 228 of 400, and it isolates "this backend's uncommitted state was consulted".
Registration is correct and alphabetical (import_deferred / index_delete_liveness /
index_fetch_penalty_width), checked because an unregistered suite is a gate that
never runs.
Gate
| arm | result |
|---|---|
| preflight PG 15/16/17/18/19 | built 5 of 5, 0 warnings |
| matrix PG18 | 232 ran, 2 skipped, ALL PASSED |
| matrix PG19 | 234 ran, 0 skipped, ALL PASSED |
index_delete_liveness |
PASS on both arms |
harness_selftest |
PASS on both arms |
| failing suites | none |
main at 808cd46 gave 231 and 233 on this box today, so +1 per arm is
index_delete_liveness actually joining the matrix rather than merely existing.
|
Follow-up to my approval, which stands — the code is right and the gate is green. I This PR fixes a bug that is live in the shipped I checked rather than assumed, because for most of this sweep the answer is the
Three of the five are alpha3-cycle regressions: introduced and fixed inside an Not asking you to re-open anything. Either a line in (My approval of #835, #837 and #841 did not raise this, and after checking, I do not |
2 participants
Closes #838. This is the most severe defect found in the bug hunt: silent, permanent loss of index entries for live rows, reachable at shipped defaults.
Same table, same session, nothing set. All 400 rows are live. A heap given the same churn answers 400 to both.
Cause
table_index_delete_tuplesasks whether an entry is dead to everyone; heapam answers fromInitNonVacuumableSnapshot(SnapshotNonVacuumable, GlobalVisTestFor(rel)). This answered from the calling backend's MVCC snapshot, a different question with a different answer. nbtree then removes the items in_bt_delitems_deletephysically, in a critical section, WAL-logged.ROLLBACKdoes not put them back.Fix
Native storage has no per-row xid to compare against a global horizon, so the callback cannot prove global deadness. It now vouches for nothing rather than guessing.
For a bottom-up caller that is free: the work is speculative by contract and
_bt_delitems_delete_checkreturns cleanly on an emptydeltidsarray (its assertion there isAssert(delstate->bottomup)). The cost is that version churn no longer reclaims leaf space on a columnar table. That is space, not answers.A simple-deletion caller has already marked its entries from LP_DEAD hints, so its marks are left as they arrived. Zeroing
ndeltidsthere would discard the caller's own findings and trip that assertion.Test, with the controls that make it specific
indexUnchangedhintChurning the indexed column makes
index_unchanged_by_update()false so no bottom-up pass runs; a heap with a second index defeats HOT and takes the same hint. Without those two arms a failure would only say that index churn loses rows in general. The suite also asserts the plan under test is an Index Scan, so a green arm cannot come from the index never being consulted.Removal proof, and a first attempt that was invalid
Deleting only the new guard leaves a third behaviour that neither marks entries nor compacts the array. That tripped
Assert(ndeletable > 0 || nupdatable > 0)and put the cluster into recovery: a red for the wrong reason, which proves nothing about row loss. Recorded because the failure text is what caught it.Restoring the original snapshot-based loop gives the right red: arm A 0 of 400, arm D 228 of 400,
count(*)228 againstcount(b)400 at defaults, both controls still clean. Installed.soece7896a10fcunfixed againstf425a69f6b87fixed.Regressions
native_index,native_delete_vector_index,native_index_projection,index_only,native_ios,native_delete_visibility_paths,native_dml,native_fetch_cache,index_fetch_penalty_width,native_index_fetch_stripe_cost,isolation,unique_conc,update_conc,concurrency: all pass unchanged.Credit
Surfaced by a reading review of the table-AM callbacks, then reproduced and bounded independently before being believed.