Repository navigation
fix(#4935): stop double-update in one tx from leaving phantom index e… - #4964
Conversation
…ntries When the same indexed record was updated more than once inside a single transaction, the eager in-transaction index update diffed each save against the record's committed buffer. Because serialization is deferred until commit, that buffer never advances during the transaction, so every save used the original committed value as its "before" image and the intermediate ADD was never cancelled. On a unique index this left a phantom entry that resolved by lookup to the final record and blocked a later legitimate insert of the intermediate value with a spurious DuplicatedKeyException; on a non-unique index it left a stale entry for every intermediate value. Reachable from ordinary SQL, e.g. "UPDATE x SET k=1; UPDATE x SET k=2" in one transaction. Track a per-RID snapshot (Document.detach()) of the last in-transaction indexed state on the TransactionContext and diff each subsequent save against it instead of the committed buffer. The first update of a record in the transaction still diffs against the committed original; later updates diff against the previous in-transaction value, so the intermediate REMOVE cancels the buffered ADD and the net buffered change is the correct original -> final. The snapshot map is cleared on transaction reset/rollback. Regression test: DoubleUpdateInTransactionIndexTest covers double and triple updates on unique and non-unique indexes, verifying no phantom entries survive and the unique constraint is not poisoned.
|
Tick the box to add this pull request to the merge queue (same as
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #4935 by tracking the last in-transaction indexed state of a record in TransactionContext and diffing subsequent updates against this snapshot instead of the committed buffer, preventing phantom index entries. Feedback recommends cleaning up this new updatedRecordsIndexSnapshot map in removeRecordFromCache and removeFile to avoid potential memory leaks or stale lookups.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| public void setLastIndexedSnapshot(final RID rid, final Document snapshot) { | ||
| if (updatedRecordsIndexSnapshot == null) | ||
| updatedRecordsIndexSnapshot = new HashMap<>(); | ||
| updatedRecordsIndexSnapshot.put(rid, snapshot); | ||
| } |
There was a problem hiding this comment.
While tracking the last in-transaction indexed state in updatedRecordsIndexSnapshot is a great fix for #4935, we should ensure that this map is properly cleaned up when records or files are removed during the transaction. Otherwise, it could lead to memory leaks or stale snapshot lookups.
Please update the following methods in TransactionContext.java (which are outside the current diff hunks):
- In
removeRecordFromCache(final RID rid):
if (updatedRecordsIndexSnapshot != null)
updatedRecordsIndexSnapshot.remove(rid);- In
removeFile(final int fileId):
if (updatedRecordsIndexSnapshot != null)
updatedRecordsIndexSnapshot.keySet().removeIf(rid -> rid.bucketId == fileId);
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 38 |
🟢 Coverage 89.47% diff coverage · -7.23% coverage variation
Metric Results Coverage variation ✅ -7.23% coverage variation Diff coverage ✅ 89.47% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (f8282b0) 134232 99762 74.32% Head commit (06768e4) 166114 (+31882) 111446 (+11684) 67.09% (-7.23%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#4964) 95 85 89.47% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Code Review — PR #4964 (fix #4935: double-update leaves phantom index entries)Solid fix with a clear root-cause writeup and a focused regression test. The core logic is correct: diffing the second+ update against a per-RID in-transaction snapshot (instead of the frozen committed buffer) makes the intermediate REMOVE cancel the buffered ADD, yielding the correct net Correctness — looks good ✅
Performance / GC — worth a look
|
…aching the whole document Addresses the substantive point of the PR review: document.detach() ran on every indexed-record update, copying every property (recursively detaching embedded documents) and retaining the copy until commit, even in the common single-update case where the snapshot is never read. A bulk UPDATE over N wide rows allocated and retained N full-document copies. updateDocument only ever reads the indexed properties of the original record, so the snapshot now stores exactly those: the stripped key names (nested paths resolved eagerly) plus, for dotted names, the root property read directly by the LIST BY ITEM delta logic. Container values are shallow-copied with embedded documents detached, so the snapshot can never alias state the caller later mutates in place. Cost per update is now independent of document width: in a 20k-record bulk update of 50-property documents the snapshot retention drops from ~130 MB to ~2 MB (~99% less), with no measurable time difference. Also addresses the review nits: the per-RID snapshot is evicted when the record is deleted in the same transaction, the duplicate getTransaction() call is hoisted, and the field-block alignment in TransactionContext is restored. New regression tests per the review's suggestions: composite index double-update, first-save-keeps-key/second-moves-it, LIST BY ITEM double-update and in-place list mutation (asserted directly against the index content, since a CONTAINS query answered by a full scan would hide a stale index), and update-then-delete in the same transaction.
|
Review: fix(#4935) double-update in one tx leaving phantom index entries Thorough, well-targeted fix. The root cause analysis is correct: because record serialization is deferred to commit, Strengths
Performance consideration (main point worth discussing) Minor notes
Note Overall this is a high-quality, well-documented fix with strong regression coverage. The main thing to validate before merge is the per-update allocation impact on large single-update batches. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4964 +/- ##
============================================
- Coverage 65.33% 65.32% -0.02%
Complexity 814 814
============================================
Files 1682 1682
Lines 134232 134311 +79
Branches 28713 28736 +23
============================================
+ Hits 87705 87733 +28
- Misses 34507 34544 +37
- Partials 12020 12034 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Addresses the review's benchmark request and closes the biggest remaining per-update cost. Measured against the pre-fix baseline (no snapshot at all) on 50k single-update transactions of narrow 3-property documents: - updates that CHANGE the indexed value: ~6% slower (median 277ms vs 262ms, ~0.3us per update). This is the eager capture the fix requires - update 1's values must be copied before the live document is mutated again - and only applies when an index entry was actually rewritten. - updates that DON'T change any indexed value (e.g. bulk-updating a non-indexed property on an indexed type): now at pre-fix parity. Since getInvolvedIndexes() selects indexes by type, not by changed properties, these previously paid the snapshot on every save. updateDocument now reports whether any index entry was actually removed/added, and the snapshot is refreshed only then: when nothing changed, the previous diff source (committed buffer or an earlier snapshot) still describes the indexed state, so correctness is unaffected. Also from the review notes: - copyForSnapshot now defensively copies Set (as LinkedHashSet, preserving equals semantics for the diff) and Object[] values instead of storing them aliased, with a comment on the expected indexed shapes. - documented that the per-RID snapshot map scales with (and never beyond) updatedRecords, with entries dropped on in-tx delete and released on reset(). New regression test: key change -> non-indexed-only change (refresh skipped, retained snapshot must stay accurate) -> key change again, which must diff against the first save's state.
…tracted key values Removes the ~6% bulk-update overhead measured against the pre-fix baseline. The snapshot was built in a second pass (buildIndexedStateSnapshot) that re-extracted every indexed property the diff had just read. updateDocument now retains the per-index new key values it extracts for the diff and, only when an index actually changed, assembles the refreshed snapshot from them directly, returning it to the caller (null when nothing changed). For the common scalar-index case the snapshot costs one map insert per property: no second extraction, no container copies. Expansion indexes keep one direct property read for the container value (the list/map itself is needed, not the expanded elements) and for the fixed properties of composite expansion indexes; unchanged standard indexes reuse their already-extracted values, since their current values equal the previous state by definition. Measured on the same 50k single-update / narrow-document workload (12 JVM samples per side): median 259ms vs 262ms pre-fix - parity within noise, down from 277ms with the two-pass snapshot. Updates that change no indexed value remain snapshot-free from the previous commit. buildIndexedStateSnapshot is removed; the behavior is covered by the same 9 regression tests (double/triple updates, composite, LIST BY ITEM deltas, in-place mutation, change/no-change/change sequence, update+delete).
Code Review - PR #4964: fix(#4935) phantom index entries on double-update in one txReviewed the diff and the final state of all touched files. Overall this is a high-quality, well-reasoned fix with excellent test coverage and careful attention to the subtle correctness and memory pitfalls. The root cause (diffing every in-tx save against a committed buffer that never advances because serialization is deferred) is correctly identified and cleanly addressed by threading a per-RID snapshot of the previous in-transaction indexed state. What I liked
Suggestions (non-blocking)
Correctness spot-checks I ran (all pass)
Note: I could not run Nice work - the fix is correct and the test suite is a model for regression coverage. |
…le point From the review's non-blocking notes: - updatedRecordsIndexSnapshot now shares updatedRecords' exact lifecycle: besides reset(), it is released at the end of the commit 1st phase (no further updateRecord can happen for the tx past that point, so large batches free the snapshots one phase earlier), on the rollback reload path, and in the test-only kill(). - documented that copyForSnapshot is intentionally ONE level deep: nested container elements are stored by reference because indexed values are scalars or flat lists/maps - a nested container cannot be an index key, and deep-copying would tax every snapshot for a shape the diff never compares.
Review: fix(#4935) — double-update in one tx leaving phantom index entriesThorough, well-reasoned fix. The root-cause analysis (the committed buffer stays frozen until commit because serialization is deferred, so every in-tx save diffs against the original) is correct, and the per-RID indexed-state snapshot is the right shape for the fix. The commit history shows a careful progression from a full Correctness — looks soundI traced the key paths and they hold up:
Notes (all non-blocking)
Style / conventionsMatches the codebase: Nice work — the fix is correct and the regression suite is genuinely convincing (asserting directly against index content for the BY ITEM cases rather than through a query that a full scan could mask is a good call). I couldn't run |
#4964) * fix(#4935): stop double-update in one tx from leaving phantom index entries When the same indexed record was updated more than once inside a single transaction, the eager in-transaction index update diffed each save against the record's committed buffer. Because serialization is deferred until commit, that buffer never advances during the transaction, so every save used the original committed value as its "before" image and the intermediate ADD was never cancelled. On a unique index this left a phantom entry that resolved by lookup to the final record and blocked a later legitimate insert of the intermediate value with a spurious DuplicatedKeyException; on a non-unique index it left a stale entry for every intermediate value. Reachable from ordinary SQL, e.g. "UPDATE x SET k=1; UPDATE x SET k=2" in one transaction. Track a per-RID snapshot (Document.detach()) of the last in-transaction indexed state on the TransactionContext and diff each subsequent save against it instead of the committed buffer. The first update of a record in the transaction still diffs against the committed original; later updates diff against the previous in-transaction value, so the intermediate REMOVE cancels the buffered ADD and the net buffered change is the correct original -> final. The snapshot map is cleared on transaction reset/rollback. Regression test: DoubleUpdateInTransactionIndexTest covers double and triple updates on unique and non-unique indexes, verifying no phantom entries survive and the unique constraint is not poisoned. * perf(#4935): snapshot only the indexed property values instead of detaching the whole document Addresses the substantive point of the PR review: document.detach() ran on every indexed-record update, copying every property (recursively detaching embedded documents) and retaining the copy until commit, even in the common single-update case where the snapshot is never read. A bulk UPDATE over N wide rows allocated and retained N full-document copies. updateDocument only ever reads the indexed properties of the original record, so the snapshot now stores exactly those: the stripped key names (nested paths resolved eagerly) plus, for dotted names, the root property read directly by the LIST BY ITEM delta logic. Container values are shallow-copied with embedded documents detached, so the snapshot can never alias state the caller later mutates in place. Cost per update is now independent of document width: in a 20k-record bulk update of 50-property documents the snapshot retention drops from ~130 MB to ~2 MB (~99% less), with no measurable time difference. Also addresses the review nits: the per-RID snapshot is evicted when the record is deleted in the same transaction, the duplicate getTransaction() call is hoisted, and the field-block alignment in TransactionContext is restored. New regression tests per the review's suggestions: composite index double-update, first-save-keeps-key/second-moves-it, LIST BY ITEM double-update and in-place list mutation (asserted directly against the index content, since a CONTAINS query answered by a full scan would hide a stale index), and update-then-delete in the same transaction. * perf(#4935): skip the snapshot refresh when no index actually changed Addresses the review's benchmark request and closes the biggest remaining per-update cost. Measured against the pre-fix baseline (no snapshot at all) on 50k single-update transactions of narrow 3-property documents: - updates that CHANGE the indexed value: ~6% slower (median 277ms vs 262ms, ~0.3us per update). This is the eager capture the fix requires - update 1's values must be copied before the live document is mutated again - and only applies when an index entry was actually rewritten. - updates that DON'T change any indexed value (e.g. bulk-updating a non-indexed property on an indexed type): now at pre-fix parity. Since getInvolvedIndexes() selects indexes by type, not by changed properties, these previously paid the snapshot on every save. updateDocument now reports whether any index entry was actually removed/added, and the snapshot is refreshed only then: when nothing changed, the previous diff source (committed buffer or an earlier snapshot) still describes the indexed state, so correctness is unaffected. Also from the review notes: - copyForSnapshot now defensively copies Set (as LinkedHashSet, preserving equals semantics for the diff) and Object[] values instead of storing them aliased, with a comment on the expected indexed shapes. - documented that the per-RID snapshot map scales with (and never beyond) updatedRecords, with entries dropped on in-tx delete and released on reset(). New regression test: key change -> non-indexed-only change (refresh skipped, retained snapshot must stay accurate) -> key change again, which must diff against the first save's state. * perf(#4935): build the snapshot inside updateDocument, reusing the extracted key values Removes the ~6% bulk-update overhead measured against the pre-fix baseline. The snapshot was built in a second pass (buildIndexedStateSnapshot) that re-extracted every indexed property the diff had just read. updateDocument now retains the per-index new key values it extracts for the diff and, only when an index actually changed, assembles the refreshed snapshot from them directly, returning it to the caller (null when nothing changed). For the common scalar-index case the snapshot costs one map insert per property: no second extraction, no container copies. Expansion indexes keep one direct property read for the container value (the list/map itself is needed, not the expanded elements) and for the fixed properties of composite expansion indexes; unchanged standard indexes reuse their already-extracted values, since their current values equal the previous state by definition. Measured on the same 50k single-update / narrow-document workload (12 JVM samples per side): median 259ms vs 262ms pre-fix - parity within noise, down from 277ms with the two-pass snapshot. Updates that change no indexed value remain snapshot-free from the previous commit. buildIndexedStateSnapshot is removed; the behavior is covered by the same 9 regression tests (double/triple updates, composite, LIST BY ITEM deltas, in-place mutation, change/no-change/change sequence, update+delete). * perf(#4935): release the snapshot map at every updatedRecords lifecycle point From the review's non-blocking notes: - updatedRecordsIndexSnapshot now shares updatedRecords' exact lifecycle: besides reset(), it is released at the end of the commit 1st phase (no further updateRecord can happen for the tx past that point, so large batches free the snapshots one phase earlier), on the rollback reload path, and in the test-only kill(). - documented that copyForSnapshot is intentionally ONE level deep: nested container elements are stored by reference because indexed values are scalars or flat lists/maps - a nested container cannot be an index key, and deep-copying would tax every snapshot for a shape the diff never compares. (cherry picked from commit 940e1c7)
…ntries
When the same indexed record was updated more than once inside a single transaction, the eager in-transaction index update diffed each save against the record's committed buffer. Because serialization is deferred until commit, that buffer never advances during the transaction, so every save used the original committed value as its "before" image and the intermediate ADD was never cancelled. On a unique index this left a phantom entry that resolved by lookup to the final record and blocked a later legitimate insert of the intermediate value with a spurious DuplicatedKeyException; on a non-unique index it left a stale entry for every intermediate value. Reachable from ordinary SQL, e.g. "UPDATE x SET k=1; UPDATE x SET k=2" in one transaction.
Track a per-RID snapshot (Document.detach()) of the last in-transaction indexed state on the TransactionContext and diff each subsequent save against it instead of the committed buffer. The first update of a record in the transaction still diffs against the committed original; later updates diff against the previous in-transaction value, so the intermediate REMOVE cancels the buffered ADD and the net buffered change is the correct original -> final. The snapshot map is cleared on transaction reset/rollback.
Regression test: DoubleUpdateInTransactionIndexTest covers double and triple updates on unique and non-unique indexes, verifying no phantom entries survive and the unique constraint is not poisoned.
What does this PR do?
A brief description of the change being made with this pull request.
Motivation
What inspired you to submit this pull request?
Related issues
A list of issues either fixed, containing architectural discussions, otherwise relevant
for this Pull Request.
Additional Notes
Anything else we should know when reviewing?
Checklist
mvn clean packagecommand