fix(compaction): preserve blob directory and manifest on offline compaction#136
Conversation
…action - carry over the blob directory from the original log directory to the working directory so that offline compaction no longer loses blob data (moved by rename by default, copied when --make_backup is specified) - reject a cross-filesystem working directory with a clear error instead of failing obscurely at the final rename - carry over the original manifest so that the instance_uuid and the persistent format version survive offline compaction - add regression tests covering blob/manifest preservation, backup-mode copy, detection of unexpectedly dropped files, byte-exact backup, and the working-dir/copy failure paths
There was a problem hiding this comment.
Pull request overview
This PR fixes a data-loss bug in offline compaction (tglogutil compaction). Offline compaction builds the compacted log in a working directory (tmp) and then replaces the original log directory wholesale via rename, so anything not explicitly carried over to tmp was deleted. Two things were being lost: the blob/ directory (blob data) and the original manifest (instance_uuid and persistent format version were reset by setup_initial_logdir). The fix adds carry-over steps for both, plus a clearer error for cross-filesystem working directories, and a comprehensive regression test suite.
Changes:
- Carry the original manifest over to
tmp(copy) soinstance_uuid/format version survive. - Carry the
blob/directory over totmp— moved byrenamein the default path, copied in--make_backupmode — with a clear error on cross-filesystem move failures. - Add regression tests covering blob/manifest survival, backup byte-exactness, no-unexpected-file-drops, and the cross-filesystem / copy-failure error paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/limestone/dblogutil/dblogutil.cpp | Adds carry_over_manifest_file, copy_directory_recursively, and carry_over_blob_directory, and wires them into compaction() (manifest before the dry-run gate, blob after it). |
| test/limestone/compaction/offline_compaction_test.cpp | Adds helpers and six regression tests covering blob/manifest preservation, --make_backup behavior, and failure paths. |
Notes for the author: One edge case is worth addressing — the docstring claims a cross-filesystem --working_dir is rejected early with a clear message, but that only applies to the non-backup move path. In --make_backup mode the blob directory is copied (which succeeds across filesystems), so the run proceeds to rename from_dir to the backup directory and then fails obscurely at the final rename(tmp, from_dir) with EXDEV, leaving no restored from_dir. Consider validating same-filesystem up front regardless of make_backup, and/or adding a cross-filesystem --make_backup test.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…pendent
- the cross-filesystem working_dir test asserted a specific error message
("failed to move blob directory"), but which cross-device operation trips
first (manifest copy, blob move, or the final rename) is platform-dependent:
CI failed at the manifest copy_file (EXDEV) before reaching the blob move
- assert only that compaction fails and leaves the original log directory
intact, not the specific message
- a cross-filesystem --working_dir made rename(tmp, from_dir) fail; with --make_backup this was especially problematic: after from_dir was renamed away to the backup, that rename failed and left from_dir not properly restored, so the database could not start even though the data itself survived in the backup (reported by Copilot review) - check that from_dir and the working directory are on the same filesystem right after the working directory is determined, before any destructive step, so both default and --make_backup modes fail with a clear message - cover the cross-filesystem rejection for both modes and assert the log directory is left byte-for-byte intact with no backup directory created
Summary
Fixes a bug where running offline compaction (
tglogutil compaction) loses blobdata. It also fixes a manifest-overwrite problem that stems from the same root
cause: offline compaction assembles a working directory and then replaces the
original log directory wholesale, but the carry-over of some entries was missing.
Related issue: https://github.com/project-tsurugi/tsurugi-issues/issues/1526
Root cause
Offline compaction builds the compacted result in a working directory
tmpand,at the end, removes the original log directory (or renames it away for backup) and
replaces it with
tmp. Anything that is not explicitly created intmpistherefore lost.
from_dir/blob/over totmp, so theblob files were deleted together with the original directory.
setup_initial_logdircreates a brand-new manifest intmp, so the originalinstance_uuidwas replaced with a freshly generated one(and the persistent format version was reset to the latest).
Online compaction does not hit this because it does not replace the directory
wholesale; it only swaps in the compacted file and leaves
blob/and the manifestin place.
Changes
from_dirtotmp.rename.--make_backup: copied, so the backup keeps its own blob data.--working_dirup front with a clear error message,in both the default and
--make_backupmodes, instead of failing obscurelylater at the final
rename(tmp, from_dir)(the rename requiresfrom_dirandtmpto be on the same filesystem).default (move) path; in
--make_backupmode the blob copy succeeds acrossfilesystems and the run failed later with
EXDEV, after the originaldirectory had already been renamed away. The check is now performed up front
regardless of
--make_backup, with regression tests for both modes.tmpso that theinstance_uuidand thepersistent format version survive offline compaction. This is safe because
main()runsmanifest::acquire_lock()andcheck_and_migrate_logdir_format()beforehand, so the manifest in
from_diris guaranteed to exist and to already bemigrated to the current format version.
--make_backupmode;to the log directory being lost the way the blob directory was);
--make_backupmodes);