HNSW Index: Refactor error handling

[villagedeb]

• Rename err()/err_len() to get_err_buffer()/ get_err_buffer_len() for clearer naming.

• Rename unused_valid and unused_num_valid to num_valid to match the documented meaning in fetch().

• Remove redundant mtr_ctx.commit() calls before failure checks; ~MtrCtx() commits idempotently.

[robgolebiowski-vsql]

LGTM

[tomas-villagesql]

Claude review:

VillageSQL Self-Review — PR #66 "HNSW Index: Refactor error handling"

Base: deb/index/6 (stacked on #65; single commit 187740f, clean merge-base — properly stacked this time). Three mechanical refactors to graph.cc/graph.h: (1) rename err()/err_len() → get_err_buffer()/get_err_buffer_len(); (2) rename unused_valid/unused_num_valid → num_valid; (3) remove mtr_ctx.commit()/sub_ctx.commit() calls that were redundant with ~MtrCtx(). Co-authored by Claude Sonnet 5 (noted). Reviewed against origin/deb/index/6.

Correctness — verified, no behavior change (the one thing that mattered)

The only behaviorally-risky item is #3 (22 net commit removals), because MtrCtx commits (never rolls back) on destruction, and I established in #56 that these are redo-only, non-transactional writes where commit ordering is load-bearing for crash consistency. So removing a commit is safe only if the destructor commits at the same logical point. I checked all 25 removals systematically:

• Every removed commit was immediately followed by scope exit — if (failed) return, return failed/false/true, or the block's closing }. I confirmed this by categorizing what followed each commit in the base: all are failure-checks or scope boundaries, none is "more mtr work in the same scope." So ~MtrCtx() fires at the identical point the explicit commit did.
• Sequential-block ordering preserved. In unlink_neighbours (and add_overflow_incoming's insert-then-link), each mtr lives in its own { } block; the destructor commits at each block's } before the next block opens — the same ordering the explicit commits gave. Verified the insert-before-link invariant in add_overflow_incoming still holds (insert's sub_ctx closes at its block } before the link-in code runs, matching the "inserted (and committed) before it is linked in" comment).
• clear_link(Overflow) sub_ctx still commits at function return (via destructor) — the HNSW Index: Replace neighbours #56-relevant timing is unchanged.
• Discriminate application confirmed: the author correctly kept the commit in unlink_neighbour (line 77), where it's followed by meaningful same-scope work (out_neighbour/orphan assignment) and must land the clear_link(Neighbour) write before the caller proceeds. This proves the removals weren't blanket — ordering-significant commits were retained.

The read-path cases (neighbours, resolve_node, get_next_level_node) move the commit to after pure in-memory post-processing (out.resize/out = Node{...}) — immaterial for a read-only mtr (commit just releases latches).

Renames — complete and safe

err()/err_len() are private IndexGraph members called only within graph.cc; no external caller breaks. Verified zero lingering err()/err_len()/unused_valid/unused_num_valid anywhere. The get_err_buffer/get_err_buffer_len names are clearer and unambiguous.

Findings

None blocking. One minor style observation:

unused_num_valid → num_valid at genuinely-unused sites (debatable, not a defect). In resolve_node, incoming_neighbours, drop_neighbour_link, etc., the renamed num_valid is a required fetch out-param whose value is still never read. The old unused_* name signaled "this caller intentionally ignores this"; the new name loses that local signal in favor of matching fetch's parameter name. Both are defensible — consistency vs. local intent. Not worth changing, but if the goal is clarity, [[maybe_unused]] size_t num_valid at the ignore-sites would capture both. (No unused-variable warning risk, since it's written by the callee via reference.)

Diff hygiene — clean

The rename touches many lines but is purely mechanical; the commit removals fold bool failed = ...; commit(); if (failed) into if (op(...)) which is a genuine simplification consistent with the new structure, not gratuitous reformatting. No line-refs, Phase narrative, or section separators introduced.

Verdict: Approve. A careful, correct cleanup — the risky part (commit removal on ordering-sensitive redo-only writes) was applied discriminately and preserves commit timing at every site; the ordering-significant commit in unlink_neighbour was correctly retained. The only note is the cosmetic num_valid naming at ignore-sites.

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}