Repository navigation
[core] Detect fixed-bucket deletion vector commit conflicts - #10700
discivigour wants to merge 8 commits into
Conversation
Carry the previous bucket DV through deletedIndexFiles for both compaction and direct DV deletes. Validate index replacements against the latest snapshot, reject rewrites that missed an input DV, and reject new DV references to files absent after applying the commit. Keep unchanged bucket indexes and allow concurrent DV changes on unrelated rewrite inputs. Use the existing index increment serialization and the common Optional conflict/rollback path without passing compact messages. Cover stale same-file, cross-file and cross-level PK updates, direct DELETE recomputation, physical rewrites, consecutive replacements, checkpointed writer state, commit retries, and serialization compatibility.
Represent DV updates as unchanged, replacement, or removal, and clear old indexes without writing empty files. Reuse file-level DV dependency checks for fixed buckets and retain bucket checks for initial additions and legacy empty indexes. Cover DV clearing, stale concurrent commits, and ordered prepared commits; remove unsupported cross-prepare aggregation handling and the restored-index count assertion. Validation: 288 focused tests passed with Maven package, including formatting and static checks.
JingsongLi
left a comment
JingsongLi
left a comment
There was a problem hiding this comment.
I would hold off merging this version because it introduces a checkpoint recovery compatibility regression. Before this change, replacing an existing bucket DV index D0 produced only ADD(D1). The new check interprets an ADD without a DELETE as “no previous DV index exists,” so replaying that legacy committable against D0 fails with expected DV file null, current DV file D0. Both versions still use commit-message serializer v14, with no marker distinguishing the two semantics. This can block recovery of a single dedicated compaction job, without any concurrent compactor; repeatedly restoring the same compact-only checkpoint cannot make progress. A focused serialized-message regression test reproduces this failure.
The stale-DV correctness issue is real, but overlapping compaction jobs appear to be a very uncommon scenario, and we already recommend a single compaction owner. Is the benefit sufficient to justify changing DV generation, cleanup, dependency tracking, and validation on ordinary commit paths, including additional index-manifest scanning? I would prefer a narrower fix, with the empty-DV-file cleanup considered separately. Before merging, we should establish the practical need, provide an explicit compatibility strategy for legacy pending checkpoints, and validate the impact on normal single-compactor workloads.
Purpose
Concurrent writers or compactors can restore the same fixed-bucket deletion vector (DV) index and prepare separate replacements. Because each replacement covers the entire bucket, a stale commit can discard another writer's deletion marks and expose duplicate primary keys. A concurrent data-file rewrite can also invalidate a DV's references without changing the bucket's DV version.
This change emits explicit DELETE entries for the previous DV index when replacing or clearing it, for both compaction and direct DV DML. Unchanged, replacement, and removal results are distinguished so clearing an index does not write an empty DV file, and intermediate compaction outputs do not become published dependencies.
For
HASH_FIXED,ConflictDetection.checkConflictsvalidates:Tests
Regression coverage includes: