Fix double counting in the sliding window index when writes happen while the index is WRITE_ONLY - #4405
Open
hatyo wants to merge 4 commits into
Open
Fix double counting in the sliding window index when writes happen while the index is WRITE_ONLY#4405hatyo wants to merge 4 commits into
WRITE_ONLY#4405hatyo wants to merge 4 commits into
Conversation
…ite-only writes Revert SlidingWindowIndexMaintainer to its pre-FoundationDB#4405 shape: it extends IndexMaintainer again, reports isIdempotent() from its delegate, and keeps its own updateWhileWriteOnly override. Declaring the maintainer non-idempotent routed write-only writes through the range-set check, which fixed the double count but paid for it broadly: the online indexer drops to SERIALIZABLE record scans for non-idempotent targets, and a multi-target build containing a sliding window index drags the whole target group down with it. Instead, make the insert itself replay-safe. Writing the entry key is a no-op overwrite when the entry is already tracked, but handleInsert always increments the window count, so re-applying an insert for a tracked entry inflates the count without adding an entry. The online indexer does exactly that whenever it builds a range holding a record a concurrent write already indexed, because IndexingBase applies build updates as a plain update(null, record) with no dedup of its own. handleInsert now reads the entry key first and, when it is already tracked, deletes it before inserting, turning the replay into a delete/insert pair that nets out to no change. Putting the check in handleInsert rather than in update() covers every caller that can be handed an already-tracked entry: the plain update() the indexer uses, updateWhileWriteOnly, and the pending-write-queue drain. It also covers the direction the old updateWhileWriteOnly preemptive delete could not: that one protected a write against a record the build had already indexed, this one also protects the build against a record a write had already indexed. The delete deliberately leaves the delegate alone. The insert that follows re-adds the entry to it, and an already-tracked entry always beats whatever the delete promoted out of overflow in its place, so it cannot be stranded on the overflow side with a stale delegate entry behind it. With handleInsert replay-safe, updateWindowWhileWriteOnly no longer needs its own preemptive delete: it cleared the entry only for handleInsert to find it absent and insert normally. Removing it also retires SW_PREEMPTIVE_DELETE_WRITE_ONLY in favour of SW_PREEMPTIVE_DELETE_BEFORE_INSERT, which is the more honest metric of the two: the old counter fired on every write whose entry key changed, whether or not an entry was actually there to delete, while the new one counts only the deletes that happen.
jjezra
requested changes
Jul 29, 2026
hatyo
force-pushed
the
sw-double-counting
branch
from
July 30, 2026 11:09
02ff45d to
0825ed8
Compare
hatyo
added
the
Run mixed-mode
jjezra
approved these changes
Aug 2, 2026
jjezra
self-requested a review
August 2, 2026 22:56
jjezra
requested changes
Aug 2, 2026
hatyo
force-pushed
the
sw-double-counting
branch
from
August 20, 2026 16:29
0825ed8 to
80a872f
Compare
jjezra
requested changes
Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Records written while a sliding window index is in the
WRITE_ONLYstate end up counted twice, once by the write itself, and again when the online indexer builds the range containing them.handleInsertwrites the entry key unconditionally, which is a harmless no-op overwrite when the entry is already there, but it always bumps the window counter regardless, so the second application adds nothing to the entries subspace while still inflating the count. The window then reports more entries than it actually holds, which throws off eviction and re-election. There was already a preemptive delete inupdateWhileWriteOnlymeant to guard this, but it only worked in one direction: it protected a write against a record the build had already indexed, and nothing protected the build against a record a write had already indexed, sinceIndexingBaseapplies build updates through a plainupdate(null, record)with no dedup of its own.The fix is to make the insert itself safe to replay, and to decide "already held" per record rather than per entry key. That distinction matters because the entries subspace is keyed by (
windowValue,primaryKey), so an entry for a record whose window value has moved looks brand new even though the record is already in the window, while the HNSW delegate keys on the primary key alone and quietly absorbs the second insert, leaving the counter drifting above the number of records the delegate actually holds. SohandleInsertnow resolves the record from the base table, which is keyed by primary key, and lets that decide which entry key the record legitimately owns: an insert for any other entry key is a replay of a version that has since been superseded, and one for the key it does own is only applied if that entry is not already there. Doing this insidehandleInsertcovers every caller that can be handed a record the window already has. the plainupdate()the indexer uses,updateWhileWriteOnly, and the pending-write-queue drain.This fixes #4404.