Transaction log deletion is database-wide on RocksDB; table-scoped requests are rejected - #618
Transaction log deletion is database-wide on RocksDB; table-scoped requests are rejected#618cb1kenobi wants to merge 5 commits into
Conversation
delete_transaction_logs_before now rejects table-scoped requests on RocksDB (HarperFast/harper#2049), and the deprecated delete_audit_logs_before always errors there since it requires table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request updates the database transaction documentation to deprecate the delete_audit_logs_before operation and introduces documentation for the delete_transaction_logs_before operation, detailing its behavior on RocksDB and LMDB. The feedback suggests adding a dedicated ### Transaction Log Operations section header to improve the document's structure and clarity.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-618 This preview will update automatically when you push new commits. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-618 This preview will update automatically when you push new commits. |
…ecords Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-618 This preview will update automatically when you push new commits. |
kriszyp
left a comment
There was a problem hiding this comment.
Probably should clarify the response, since it is returns a job-id.
🤖 Reviewed with Codex
…ment, type style Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-618 This preview will update automatically when you push new commits. |
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
I went through this against the merged implementation in harper#2064 rather than taking the prose on trust — the 400 guard for a RocksDB table scope, the engine-agnostic 404 that sits before the engine branch, cleanup_deleted_records against Table.deleteHistory, the background-job dispatch, and the response body all check out line by line. Every factual claim in the diff is accurate, and all eight points from the earlier rounds (Gemini's section header, @kriszyp's five Codex findings, Barber AI's two) are correctly resolved in ffe30c44. I'm not re-raising any of those.
What's left is what the page doesn't say, which on a destructive operation is the part that matters. Four gaps:
-
On LMDB, omitting
tableis a silent no-op — and the section's only example omits it. That request resolvesgetTable()toundefined, falls into theif (!table)branch, finds noRocksDatabaseinstance, and returnsentries_deleted: 0with job statusCOMPLETE. An LMDB operator copying the example verbatim schedules a retention job that quietly does nothing and reports success. Harper's own validator acknowledges this (validation/transactionLogValidator.ts:20:// \table` will need to be required for lmdb, but not for rocksdb`). Inline suggestion below. -
Requires clustering: Yesin the comparison table is false on v5/RocksDB. The 5.0 release notes describe the transaction log as Harper's "own native transaction log as a write-ahead log (WAL) for RocksDB" — unconditional, not a clustering feature;readTransactionLogjust delegates toharperBridge.readAuditLogwith no stream involvement; andreference/backups/overview.mdsays every managed backup includes the transaction log with no clustering caveat. As it stands the row directly contradicts the Storage row above it, which this PR rewrote. A RocksDB reader concludes they have no transaction log unless they've configured replication, then can't reconcile that with a page telling them how to delete it. Inline suggestion below. (TheAvailable since v4.1.0row has the same v4-era problem, but I'd leave that one for a separate pass.) -
A database-wide destructive purge is documented in plain body prose. Line 146 is the sentence that tells an operator who wanted to prune one table to instead purge every table's history, and it sits in an unmarked paragraph. The reference tree uses admonitions for exactly this (
:::warning Data safetyinreference/database/api.md:54). Three things are missing beyond the wrapper: what is actually lost (purgeLogs()deletes whole log files — no per-table survivor, no undo), and the recovery route (a managed backup restores the transaction log alongside the data, perreference/backups/operations.md:113). Inline suggestion below covers both.The third piece I couldn't establish myself and it's a genuine question for you or @kriszyp: what happens if you purge below a lagging peer's replication position?
purgeLogslooks like a whole-file delete with no watermark check at the bridge layer. If there's no guard, "do not purge below your slowest replica's position" belongs in that admonition. If rocksdb-js refuses to delete unshipped files, that's reassuring and also belongs there. Either answer improves the page — silence is the only bad option, so I left it out of the suggestion rather than guess. -
No entry in
release-notes/v5-lincoln/5.2.md. This is breaking in three ways: table-scopeddelete_transaction_logs_beforenow errors where it previously returned success;delete_audit_logs_beforenow always errors on the default engine; and a nonexistenttablenow 404s on LMDB where it previously returned a clean success. Anyone with a scheduled retention job hits one of the first two on upgrade.5.2.mdis actively maintained with prose sections and a reference-page badge isn't a substitute. A short### Transaction Log Deletionsection would do it. Fine as a follow-up if you'd rather not grow this PR — but it shouldn't ship without one.
Everything else below is a suggestion, take or leave. Items 1-3 are cheap enough that I'd land them here.
sent with Claude Opus 5
…fety admonition, params/results, get_job error example, 5.2 release note Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@Ethan-Arrowood thanks for the verification-grade pass. All six inline threads are addressed in 5cc6c51 (replies on each), and gap 4 landed in this PR rather than as a follow-up: release-notes/v5-lincoln/5.2.md now has a "Transaction Log Deletion" section covering all three breaking behaviors (RocksDB table-scope rejection, delete_audit_logs_before always failing on RocksDB, nonexistent-table 404 on either engine) with a link to the updated reference page. Your replication-lag question turned out to be answerable — no purge-side guard exists, but the sender forces a bounded base copy when a peer's requested start predates retained history (shouldForceBaseCopyForRetention, harper-pro#277), so the admonition now states the real cost: full resync, not data loss. — Claude (Fable 5) |
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-618 This preview will update automatically when you push new commits. |
Companion to HarperFast/harper#2064 (fixes HarperFast/harper#2049 — RocksDB: table-scoped delete_transaction_logs_before destroys the entire database's transaction log).
Updates
reference/database/transaction.md:delete_transaction_logs_beforesection: on RocksDB (the default engine) deletion is database-wide because all tables in a database share one transaction log; as of v5.2.0 a request naming atableis rejected with a 400 (previously the table scope was silently ignored and the whole database's log was purged), and a nonexistenttablereturns a 404. On LMDB,tablescopes deletion to that table, unchanged.delete_audit_logs_beforethat it is deprecated in favor ofdelete_transaction_logs_before, and that on RocksDB it always errors since it requirestable.npm run buildandnpm run format:checkpass locally.Generated by Claude (Fable 5).
🤖 Generated with Claude Code