fix: try removing old sstables in recovery - #173
Conversation
There was a problem hiding this comment.
Pull request overview
Implements the previously noted recovery TODO by attempting to remove obsolete SSTables during manifest recovery, reducing leftover on-disk files after a crash between manifest update and file deletion.
Changes:
- Capture
files_to_removefromapply_compaction_resultduring recovery. - Attempt to delete obsolete
.sstfiles during manifest replay (ignoringNotFound) in both the non-MVCC and MVCC storage engines.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| mini-lsm/src/lsm_storage.rs | During manifest recovery, deletes SSTs that were compacted away according to recovered compaction records. |
| mini-lsm-mvcc/src/lsm_storage.rs | Mirrors the same recovery-time obsolete SST cleanup logic in the MVCC variant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ManifestRecord::Compaction(task, output) => { | ||
| let (new_state, _) = compaction_controller | ||
| let (new_state, files_to_remove) = compaction_controller | ||
| .apply_compaction_result(&state, &task, &output, true); | ||
| // TODO: apply remove again | ||
|
|
||
| // try removing old ssts | ||
| for table_id in files_to_remove { | ||
| if let Err(err) = | ||
| std::fs::remove_file(Self::path_of_sst_static(path, table_id)) | ||
| { | ||
| if err.kind() != std::io::ErrorKind::NotFound { | ||
| return Err(err.into()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think that's necessary here
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Thanks for tackling the recovery cleanup. This is not safe to merge as-is: in mini-lsm-mvcc, any obsolete-SST deletion error other than NotFound aborts open(), while the non-MVCC implementation treats cleanup as best-effort. An inability to delete an already-obsolete file should not prevent database startup. Please make both implementations best-effort, and add a restart regression test that verifies obsolete SSTs are cleaned up after a completed compaction. It would also be safer to perform cleanup only after manifest replay and validation/opening of the current live SST set have succeeded. 🤖 (posted by Codex) |
|
Done @skyzh |
just implemented a simple TODO