feat(dotc1z): opt-in rebuild cleanup strategy (clone keep-set, repoint) - #912
feat(dotc1z): opt-in rebuild cleanup strategy (clone keep-set, repoint)#912arreyder wants to merge 4 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| for _, suffix := range []string{"", "-wal", "-shm"} { | ||
| _ = os.Remove(oldPath + suffix) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: After removing the old db files, the parent directory is leaked as an empty dir. The rest of the codebase uses cleanupDbDir (which calls os.RemoveAll(filepath.Dir(...))) to clean up temp directories holding db files. Consider adding _ = os.Remove(filepath.Dir(oldPath)) after the file removals, or reusing cleanupDbDir, to avoid accumulating empty temp dirs across rebuilds.
General PR Review: feat(dotc1z): opt-in rebuild cleanup strategy (clone keep-set, repoint)Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryReviewed the full PR: a new opt-in Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
Alternative to per-table DELETE + VACUUM cleanup. When WithCleanupRebuild (or BATON_CLEANUP_REBUILD) is set, Cleanup copies the kept syncs into a fresh database (reusing CloneSync's column-explicit ATTACH copy) and repoints the C1File at it, then drops the old file. Cost scales with the kept set instead of the deleted set, and the result is compact in one step with no VACUUM. Benchmarked ~11x faster than DELETE+VACUUM on a 1M-grant fixture (1.7s vs 18.5s) and self-compacting. Default cleanup path is unchanged; this is opt-in. Keep selection reuses SelectSyncsToDelete (same policy). Test: TestCleanupByRebuild proves survivors' data is byte-identical and the repointed db passes integrity_check; full pkg/dotc1z suite passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rip test - Repoint now calls c.init() on the new handle so session pragmas (locking_mode=EXCLUSIVE, synchronous, journal_mode, caller pragmas) match a normally-opened C1File — previously the bare sql.Open handle had none, which could break finalize's WAL checkpoint under WAL journal mode. - On close/open failure during repoint, nil out rawDb/db so a later Close() takes the safe nil path instead of double-closing / removing backing data. Set rebuilt before init so a pragma failure can't delete the adopted db. - Nil initFile handles after close (matches CloneSync). - Doc the keep-set copy as an O(syncLimit) transaction (scaling caveat). - Test: reopen the c1z from disk after Close to prove the repointed db round-trips through saveC1z; add no-op (nothing-to-delete) path test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…alidated cols) go-lint flagged G201 (SQL string formatting) on the per-table rebuild INSERT. Table names come from the hardcoded allTableDescriptors and columns are validated by cloneTableColumns/validateColumnName — same justification and //nolint:gosec directive already used in c1file_attached.go. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The repoint removed the old db/-wal/-shm files but left the parent temp dir as an empty leak. Reuse cleanupDbDir (os.RemoveAll on filepath.Dir), matching finalize's cleanup, so rebuilds don't accumulate empty temp dirs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dacfb05 to
cc02918
Compare
1 participant
What
An opt-in alternative to the per-table
DELETE + VACUUMcleanup. WhenWithCleanupRebuild(true)(orBATON_CLEANUP_REBUILD=1) is set,Cleanupinstead:SelectSyncsToDeletepolicy — keep current + N most recent).CloneSync's column-explicit ATTACH copy).C1Fileat the compact db and drops the old file.Cost scales with the kept set (small), not the deleted set, and the result is compact in one step — no VACUUM.
Why (vs #911)
This is the other half of the cleanup investigation. From the autoresearch benchmark (1M-grant old generation + 100k keep):
DELETE + VACUUM(current default)DELETE(PR #911)The two are independent and not mutually exclusive: #911 can land for everyone while this rolls out behind the flag.
Risk / review focus
c.rawDb.Close()→ open fresh handle on the new path → swapc.db) is the sensitive part — blast radius is the c1z save path. Please scrutinize the handle/lifecycle swap and the WAL/file cleanup of the old db.idcolumn (fresh rowids) exactly like the existingCloneSync;external_id/sync_idare the stable keys.Test
TestCleanupByRebuild: records each sync's grant count before cleanup, then asserts only the policy-kept syncs survive, their grant counts are byte-identical afterward (no data loss), and the repointed db passesPRAGMA integrity_check.pkg/dotc1zsuite passes;gofmt+vetclean.Not yet wired into the platform caller; that's a follow-up once a strategy is chosen.
🤖 Generated with Claude Code