fix(posting): mark removed split parts for reclamation - #9825
KeisukeYamashita wants to merge 1 commit into
Conversation
d5295a5 to
5e99229
Compare
5e99229 to
5475bc3
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthrough
ChangesPosting split cleanup
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Merge Risk: 🟡 Moderate · up to Posting rollups now write empty records for removed splits to enable reclamation. If split validation rejects this representation, deletion rollups could fail; the invariant should be confirmed before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
||
| // Retire removed parts at the parent's timestamp (+1 for finite readTs) | ||
| // so compaction preserves reads using the old parent. | ||
| for _, startUid := range l.plist.Splits { |
There was a problem hiding this comment.
The crash-safety of this depends on a write-ordering invariant that's currently implicit. If one of these tombstones ever became durable before the new parent KV and the process died in between, the old parent would still be the latest visible version and would reference a part that now reads as empty. Worse, the next rollup would read that part as empty and persist the loss.
Today the ordering holds: the sort below puts the main key first (DefaultPrefix 0x00 sorts before ByteSplit 0x04), TxnWriter submits commits in call order, and badger's WAL replays a strict prefix, so a tombstone can't survive a crash without its parent. But nothing in the code says that order is load-bearing (the sort comment only mentions readability). Could you add a short comment here or on the sort noting that the main-list KV must be written before these tombstones?
| if err != nil { | ||
| return nil, errors.Wrapf(err, "cannot marshal removed posting list part") | ||
| } | ||
| part.Version = kv.Version |
There was a problem hiding this comment.
This works only because the retained-parts loop above shadows kv in its own scope, so the outer kv here is still the main-list KV with the +1 applied. That's fragile: a refactor of that loop could silently change which version these tombstones get. Suggest capturing parentVersion := kv.Version right after it's computed and using it here.
While you're at it, a line on why removed parts get the parent's +1 version while retained parts stay at out.newMinTs would help future readers: a reader at exactly newMinTs still resolves the old parent, which must still see the removed part's data.
2 participants
Refs #9824
Description
Write
BitEmptyPostingrecords for split parts removed during rollup, allowing Badger compaction to reclaim their old payloads.Use the replacement parent's timestamp, including the
+1for finite read timestamps, to preserve historical reads.Add regression tests covering:
Validation: the posting test suite, targeted race tests (three runs), targeted debug tests, and Dgraph build pass. Trunk has not yet been run.
This change does not remove empty-key metadata or retroactively clean up already-orphaned parts.
Checklist
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
Bug Fixes
Tests