chore(kv): elide redundant index rewrites on identical in-place updates (#279)

[EnRaiha]

chore(kv): elide redundant index rewrites on identical in-place updates

Fixes #279

Problem

An in-place update that left an indexed column unchanged still removed and re-inserted the entry, at two index writes per index per PUT. Both writes were correct, so this is churn rather than a defect.

Change

Single-field and composite indexes now skip both operations when the extracted bytes are unchanged AND the (value, primary key) pair is already present. The membership gate matters: a row written before a backfill=false registration is absent from the index, and the next identical PUT has to file it in rather than elide it.

Changed values keep the existing remove+insert path. Sorted indexes are untouched — OrderStatTree::insert already returns early on an identical sort key.

How to test

cargo nextest run -p nodedb --lib -E 'test(engine::kv::index) or test(wal_replay_kv_index)'

Covers the KV index unit tests, the three new elision tests (identical_update_elides_index_writes, plus the backfill-absent insert cases for single-field and composite indexes), and the four wal_replay_kv_index backfill tests.

Verified

• cargo fmt --all -- --check — clean
• cargo clippy -p nodedb --all-targets --all-features -- -D warnings — clean
• bash scripts/ci/check_calvin_determinism.sh — clean as submitted at c7a41c4
• nextest run above — 25 pass

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[farhan-syah]

Request changes. The elision is correct only when the index already holds the entry. It does not always hold it.

Blocker

set.rs:172 skips remove+insert when the old and new indexed bytes are equal. That assumes (value, primary_key) is already indexed. A row that predates a backfill=false registration is not indexed.

KvFieldIndex::entries states the case: backfill=false omits pre-existing rows. On main, the next PUT of such a row files it in — remove no-ops, insert adds it. With this patch, a PUT that leaves the field unchanged is elided and the row never enters the index. lookup_eq misses it permanently.

wal_replay_kv_index.rs:279 asserts this guarantee. It still passes only because it writes a new key, not a rewrite.

Measured, not inferred. Probe added to the KvIndexSet tests:

let mut set = KvIndexSet::new();
set.add_index("status", 0);                                    // k1 predates the index
set.on_put(b"k1", &[("status", b"active")], Some(&[("status", b"active")]));
assert_eq!(set.lookup_eq("status", b"active").len(), 1);

• main: PASS
• this branch: FAIL — left: 0, right: 1

Fix: gate the skip on membership, not equality. Add KvFieldIndex::contains(value, pk) — a BTreeMap lookup plus a BTreeSet lookup, O(log n), no allocation. Skip only when the bytes match and the pair is present. The hot path is present-and-unchanged, so the win survives: one tree probe replaces two Vec allocations. Apply the same gate to the composite index.

Should-fix

Item	Where	Action
Branch duplication	set.rs:185	The else arm re-implements the insert half. Under a contains gate both arms collapse into one loop, with old_val = None for a new row.
.find() vs loop	set.rs:167	The update arm takes the first match per field, the insert arm inserts every match. extract_all_field_values_from_msgpack emits one pair per key, so behavior is unchanged today. The two arms must not disagree on the contract. Unification removes it.
Test coverage	set.rs tests	d6_identical_update_elides_index_writes covers the entry-present single-field case only. Add the entry-absent case and a composite identical-tuple case.
Test command	PR body	Use cargo nextest run, not cargo test. The nextest test-group config does not apply otherwise.

Verified

• 23 index_set tests and d6_identical_update_elides_index_writes pass on this branch. Your claim holds.
• Removed-field remove-only and added-field insert-only paths preserved.
• Composite filter_map length guard carried through unchanged.
• One file, scoped to the issue, no unrelated edits.

[farhan-syah]

Blocker. This assumes the index already holds (value, primary_key). A row written before a backfill=false registration is absent from the index. On main the next PUT files it in — remove no-ops, insert adds it. Here that PUT is elided and the row never becomes visible to lookup_eq.

Verified: a probe asserting the row is indexed after an identical rewrite passes on main and fails here (left: 0, right: 1).

Gate the skip on membership instead. Add KvFieldIndex::contains(value, pk) — BTreeMap plus BTreeSet lookup, O(log n), no allocation — and skip only when the bytes match and the pair is present. The common case stays present-and-unchanged, so the optimization keeps its win.

[farhan-syah]

Same shape as set.rs:172: equality alone does not prove the composite entry is present. No production path registers a composite index today, so this is latent, not live. Fix it under the same contains gate in this pass.

[farhan-syah]

Request changes. The round-1 blocker is fixed correctly. The contains gate plus d6_backfill_absent_pair_still_gets_inserted and d6_composite_backfill_absent_pair_still_gets_inserted close the backfill=false gap on both index kinds. Two things block merge.

Verified on the branch, not taken on trust:

• cargo nextest run -p nodedb --lib -E 'test(index_set) or test(d6_)' — 26/26 pass.
• The elision is semantically safe. KvFieldIndex::insert files into a BTreeSet, so remove+insert of an identical pair was already idempotent. The win is two to_vec() allocations per index per PUT plus an honest write-amp number, not a correctness change.
• The sorted-index claim holds. OrderStatTree::insert returns early when the sort key is unchanged (sorted_index/tree.rs:72).

Blockers

1. cargo fmt --all -- --check is red — set.rs:520 and set.rs:533. Verified on the branch. Clippy is unverified — run it locally before you resubmit.
2. Both debug_assert! guards are tautological — set.rs:177 and set.rs:239. Detail inline.

Should fix

3. The composite value extraction is written three times — set.rs:216, 226, 263. Detail inline.
4. A claim in the PR body is wrong. "BTreeMap + BTreeSet lookup, no allocation" does not hold for composite: KvCompositeIndex::contains calls build_key, which allocates a Vec<u8> per call. It still beats the remove+insert it replaces. Correct the wording.

Notes, no change requested

• Scope. The Calvin marker commit is unrelated to #279, and it is now handled on main directly. Drop that commit and rebase — see the inline note.
• Commit message. The last commit names an internal branch and "option B". Drop that when you amend for the fmt fix.
• write_amp_ratio (engine_index.rs:155) now reports mutations performed rather than maintenance ops issued. That is the point of the change. Flagging it because the numbers it exposes will drop.
• The same allocation win is still available on the sorted path: sorted_index/manager.rs:193 builds a sort key and a primary_key.to_vec() on every PUT, then insert discards both when the key is unchanged. Out of scope here.

Resubmitting

Squash to one commit before the next push. Four commits — a fix, a review fix, a refactor of that fix, and a CI marker — is more history than one chore needs, and the middle two only describe review rounds.

git fetch origin
git reset --soft $(git merge-base HEAD origin/main)
git commit            # write the single message below
cargo fmt --all
cargo clippy -p nodedb --all-targets -- -D warnings
cargo nextest run -p nodedb --lib -E 'test(index_set) or test(d6_)'
git push --force-with-lease

Suggested message:

chore(kv): elide redundant index rewrites on identical in-place updates

An in-place update that left an indexed column unchanged still removed
and re-inserted the entry, at two index writes per index per PUT. Both
writes were correct, so this is churn rather than a defect.

Single-field and composite indexes now skip both operations when the
extracted bytes are unchanged AND the (value, primary key) pair is
already present. The membership gate matters: a row written before a
backfill=false registration is absent from the index, and the next
identical PUT has to file it in rather than elide it. Changed values
keep the existing remove+insert path. Sorted indexes are untouched --
OrderStatTree::insert already returns early on an identical sort key.

Fixes #279

[farhan-syah]

This can never fire. The condition duplicates the if on the line above, and nothing mutates the index between the two. It is always true when reached.

The commit message says these guards "catch backfill regression in debug/CI". They cannot. In a debug build they only repeat the BTreeMap plus BTreeSet lookup.

Remove it. d6_backfill_absent_pair_still_gets_inserted is the real guard against that regression, and it works.

[farhan-syah]

Same defect as the single-field guard: the condition duplicates the enclosing if, so it can never fail.

Worse here. KvCompositeIndex::contains calls build_key, so a debug build allocates a second key Vec to re-prove something already known.

Remove it. d6_composite_backfill_absent_pair_still_gets_inserted covers the case.

[farhan-syah]

This filter_map plus find block appears three times in one function — here, at line 226, and at line 263. Only the source slice differs.

Hoist it to one helper, fn composite_vals(ci: &KvCompositeIndex, values: &[(&str, &[u8])]) -> Vec<&[u8]>. Then the None arm collapses to two lines and the Some arm loses a nesting level. on_put is now around 90 lines at five levels of nesting; the helper takes most of that back.

[farhan-syah]

cargo fmt --all -- --check rewrites this and line 533 into a multi-line assert_eq!. Run it before you resubmit.

[farhan-syah]

build_key allocates a Vec<u8> on every call, so this is not the allocation-free lookup the PR body claims. The code is fine — it still costs less than the remove+insert it replaces. Correct the description.

[farhan-syah]

No change needed here — drop this commit and rebase. The marker is going onto main directly, so this file falls out of the PR.

For the record, the root cause sits in the gate script rather than in this file. check_calvin_determinism.sh anchors its skip on /^#\[cfg(test)\]/, so an indented #[cfg(test)] on an item is never recognised. Every test-only helper added under a scanned path keeps tripping the gate. That fix is worth its own PR.

[farhan-syah]

Request changes. The logic is correct. What blocks merge is the record, not the code.

Round-2 scorecard

Point	Status
cargo fmt --all -- --check red	Fixed
Two tautological debug_assert! guards	Fixed, both removed
Composite extraction written three times	Fixed, hoisted into composite_vals
"no allocation" claim wrong for composite	Open
Drop the unrelated marker commit	Done
Squash before resubmit	Open, three commits

Verified on the branch

• cargo fmt --all -- --check — clean.
• cargo clippy -p nodedb --all-targets --all-features -- -D warnings — clean.
• cargo nextest run -p nodedb --lib -E 'test(index_set) or test(d6_) or test(kv_index)' — 30 pass, including all four wal_replay_kv_index backfill tests.
• The membership gate is correct on both index kinds. Every single-field branch matches main except present-and-unchanged, which is the elision.

Blockers

1. The PR body describes code that is no longer here. It says the skip fires "when equal" and never mentions the membership gate. How to test still runs cargo test, which ignores the nextest test-group config. The "BTreeMap + BTreeSet lookup, no allocation" line in commit 03f4d7f is false for composite: KvCompositeIndex::contains calls build_key, which allocates. A squash-merge makes this body the permanent record. Rewrite it to match the code.

2. Four comments narrate the review, not the code — set.rs:172, set.rs:221, set.rs:380, set.rs:484. Details inline.

Should fix

3. Empty if arm at set.rs:228. Inline.

4. Squash to one commit. The middle two commits describe rounds of this review.

Notes, no change requested

• The update arm still uses .find() where the insert-only arm loops. I raised it in round 1 and dropped it in round 2. Not reopening — extract_all_field_values_from_msgpack emits one pair per key, so the arms cannot disagree.
• The composite None arm repeats the else if new_vals.len() == ci.fields().len() arm. Collapsible, not worth a round.

Resubmitting

git fetch origin
git reset --soft $(git merge-base HEAD origin/main)
git commit
cargo fmt --all
cargo clippy -p nodedb --all-targets --all-features -- -D warnings
cargo nextest run -p nodedb --lib -E 'test(index_set) or test(d6_)'
git push --force-with-lease

Reuse the commit message from my round-2 review. It already describes the membership gate. Give the PR body the same content.

Last round. Fix these four and it merges.

[farhan-syah]

Drop (issue #279). Same at line 221. An issue number is provenance, not an invariant. It goes stale when the tracker moves, and a reader here cannot follow it.

Keep the rest. It states why equality alone is not enough, which is what this code needs.

[farhan-syah]

Empty if arm with the work in the else. Invert it:

if !ci.contains(&new_vals, primary_key) {
    ci.insert(&new_vals, primary_key.to_vec());
    writes += 1;
}

Same shape as the single-field arm above. Drops a level from a block already five deep.

[farhan-syah]

CLAIM D6: and the description of the old behavior address a reviewer, not the next reader. After merge there is no "unconditionally removes old + inserts new" to contrast against.

State what the test pins:

// An in-place update that leaves the indexed value unchanged must not
// rewrite the index when the pair is already present.

[farhan-syah]

Remove Farhan's blocker:. Who found the case is not a property of the case. The name also dates the comment, and it reads as if the invariant holds because someone asked.

Drop the first three words:

// old == new alone is not enough to elide — the pair must actually be
// present. A row written before a backfill=false registration was never
// indexed, so the next identical PUT has to insert it.