[matt-welch]

Description

A checkpoint save writes a vamana index in three separate steps: SVSSaveIndex overwrites the graph/data/config files in place, VamanaSaveTidMapAtomically replaces the TID map via temp-and-rename, and VamanaMarkIndexSaved writes the metapage last. A crash between the first two steps left a newer graph beside an older TID map and metapage, and the loader accepted that mismatched state with no error, silently serving a large share of the index's rows as missing. This was reproduced both offline (splicing files from two confirmed-clean checkpoint generations) and with a real crash (killing the worker with SIGKILL while it was parked mid-save), losing roughly a third of the table's rows with nothing logged.

The fix stamps the same generation value into three places as a save progresses: a new save-directory marker file written before the graph write begins, the TID map header, and the metapage. On load, all three must agree; any disagreement is treated as a torn save and the index is rebuilt from the heap instead of being served stale or partial. Writing the marker before the graph write (rather than after it completes) is what closes the window completely: a crash at any point from there through the end of the save leaves the marker already at the new generation while something else is still at the old one, which the generation check catches.

A simpler design that only cross-checks the TID map against the metapage would miss this: by write order, the metapage's generation is always at or behind the TID map's, so those two can agree with each other even when the graph itself is ahead. The marker file is what makes the graph's own progress visible to the check.

Because a mismatch discards the whole metapage rather than individual fields, a stale nextExternalId left over from a torn save can never be read back either.

Two injection points were added to VamanaSaveIndexToDisk (vamana-save-before-tidmap-write and vamana-save-before-mark-saved) so the crash test can hold the worker at the two places a crash can land. The orphaned-temp-file cleanup for the new marker checks the file through an open descriptor rather than by path, so the check and the delete refer to the same file.

Related Issues

Type of Change

• Bug fix
• New feature
• Refactor / code cleanup
• Documentation update
• Test addition or update
• Build / CI change

Pre-Merge Checklist

Build

• make completes without errors or warnings
• make install completes successfully

Tests

• If this PR introduces no new behavior: existing regression tests (make installcheck) and TAP tests (test/t/) pass with no failures
• If this PR introduces new behavior: test cases covering it were added to test/sql/ and/or test/t/
• If this PR adds a standalone unit-test module under test/modules/: it builds and passes (make -C test/modules/<module> installcheck)

Documentation

• Relevant docs under docs/ updated if architecture or usage changed

Testing Notes

Full TAP suite: 65 files, 1465 tests, all passing (only the pre-existing enable_slow_tests skip on the large-offset test). SQL regression suite: all files passing. The hardware-dependent targets also pass on a machine that supports them: prove_installcheck-hw (the two compression TAP files, 55 tests) and the halfvec_compression and vector_compression regression tests. The persistence compression test matters here because it saves and reloads real LeanVec and LVQ compressed indexes.

Two new TAP tests cover the fix directly:

• test/t/75_torn_graph_offline.pl reproduces the torn state offline by splicing a TID map (and separately, the generation marker) from an older save generation onto a newer one, and confirms a clean restart, a mismatched TID map, a mismatched marker, and an absent marker (standing in for a save made before this fix existed) are all rejected and rebuilt with full recall.
• test/t/76_torn_graph_natural_crash.pl attaches a wait injection point at the two places a real crash can land inside the save sequence (before the TID-map write, and before the metapage update), kills the parked worker with SIGKILL, and confirms both are now detected and rejected rather than served. It needs a server built with --enable-injection-points, which CI already uses, and no debugger or elevated privileges.

test/t/64_vamana_checkpoint_durability.pl was updated for the TID map header's new size (the generation stamp adds 8 bytes).

To confirm both new tests actually exercise the defect, the generation checks were disabled and both tests were rerun. The offline test showed 35-45 of 80 rows missing from every query vector with no error logged. The crash test's K1 case (killed before the TID-map write) returned noticeably fewer rows than a sequential scan with nothing logged, and its K2 case failed the check that the torn save is reported. K2 loses no rows on its own, since the TID map and graph are already current at that point and only the metapage lags. The checks were then restored and the full suite rerun clean.

[asonje]

The marker-before-graph-write ordering is the right call, and the argument for why a TID-map/metapage cross-check alone would miss this is convincing.

One thing worth considering before this lands: how the generation check behaves
on a standby.

saveGeneration is in the metapage so it replicates; generation.bin and tidmap.bin are plain files so they don't. On a standby built from a basebackup carrying a save directory, the marker is frozen at backup time while the metapage keeps advancing by replay — so the first primary checkpoint after the backup makes the two disagree permanently.

With #58 merged that's no longer dangerous: the mismatch deletes the save directory, and the next load falls through to the heap rebuild without attempting the WAL write. But two things follow:

• The standby logs WARNING: ... torn save detected, will rebuild on a healthy cluster. Nothing tore — the stamps differ because one of them replicates and two don't. this standby will emit a corruption-shaped warning every single time this index reloads,
  forever, by design.

• The check runs after SVSLoadDynamicIndex returns, so a standby deserialises
  the whole graph, frees it, deletes the directory, and rebuilds from the heap. #54's slot check was going to discard that copy regardless.

Both point the same direction as #58 did: the save directory isn't doing any work on a standby, and arguably shouldn't be in the basebackup at all. Separate change, but this is now the third PR to run into it.

Minor: vamanaio.c:871 as the K2 breakpoint in 76_torn_graph_natural_crash.pl will drift on any edit to that file, and the "breakpoint hit" assertion would still pass while parked somewhere else. A function-entry target would be more durable.

[asonje]

Two issues found, verified empirically rather than just read.

1. 76_torn_graph_natural_crash.pl's K2 case is broken, and nothing in CI has ever caught it

K2's breakpoint target, 'vamanaio.c:871', can't resolve — not intermittently, unconditionally. svs.so is built with no debug symbols at all (confirmed directly: gdb's info sources on the live worker reports (Objfile has no debug information.) for svs.so). Reproducing the exact gdb sequence by hand:
No source file named vamanaio.c.
Make breakpoint pending on future shared library load? (y or [n]) [answered N; input not from terminal]
No breakpoint is ever set. The scripted kill -9 then fires at whatever arbitrary point continue next returns control (I observed it stop on a stray SIGUSR1, unrelated to the save sequence), not the intended "after TID-map rename, before metapage update" window. On this box the result was 3 of 23 subtests failing ('k2: gdb reported the breakpoint hit', 'k2: worker process confirmed dead', 'K2: the crash-torn generation is reported and rejected').

Fix needs two changes together, verified independently:

• Change the K2 target from 'vamanaio.c:871' to 'VamanaMarkIndexSaved' (matching K1's working convention — symbol-table breakpoints don't need debug info, file:line ones do).
• Mark VamanaMarkIndexSaved pg_noinline. Without it, even the corrected name-based breakpoint still fails: it's a small static function called from exactly one place, so GCC fully inlines it away under normal optimization (confirmed via nm: symbol present with the attribute, absent without it).

With both applied: 76 goes from 3/23 failing to 23/23 passing, in 14s instead of wasting 60s waiting on a breakpoint that could never fire. 75 stays 57/57.

Separately: this is the first gdb-based test anywhere in this suite (checked every other test/t/*.pl, on this branch and main — zero other matches), and no CI workflow provisions gdb — not build-and-test.yml, not assert-build.yml either (which doesn't run on PRs at all, and wouldn't have gdb even on its weekly schedule run). So "all CI checks passed" here means literally nothing about K1 or K2: the job that runs the TAP suite reports test/t/76_torn_graph_natural_crash.pl ... skipped: gdb is not available and moves on. This bug could sit unnoticed indefinitely unless someone happens to run the suite on a box that actually has gdb + passwordless sudo.

2. Generation stamps diverge permanently on a standby built from a basebackup that includes a save directory

saveGeneration lives in the metapage, which is WAL-logged (GenericXLogFinish) and so replicates continuously. generation.bin and tidmap.bin are plain files — never WAL-logged, frozen at whatever a base backup captured. The primary's very next checkpoint after that backup advances the replicated metapage generation past what the standby's own frozen files say, and they disagree from that point on.

Confirmed this isn't self-healing: ShouldCheckpoint has a pre-existing guard — "A standby cannot persist (no WAL in recovery); it never checkpoints" — so there's no path back to a consistent state short of promotion. Every load attempt on that standby for that index, forever, hits the mismatch branch and logs "torn save detected, will rebuild" — a corruption-shaped warning on a cluster where nothing tore.

It's not dangerous — checked both callers of VamanaCheckSaveGeneration (vamanascan.c, vamanaworkerwrite.c) and neither attempts a WAL write on mismatch, so PR #58's crash-loop hazard doesn't reopen — but it's a real, permanent, misleading operational signal on any standby built this way, and nothing in the current two commits addresses it.