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 3 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>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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
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
|
|
||
| On RocksDB (the default storage engine), deletion is database-wide: all tables in a database share one transaction log, so omit `table` and pass only `database` (or `schema`) and `timestamp`. | ||
|
|
||
| <VersionBadge type="changed" version="v5.2.0" /> — On RocksDB, a request that includes `table` is now rejected with a `400` error; previously the `table` scope was silently ignored and the entire database's transaction log was purged. On either engine, a `table` that does not exist now returns a `404` (previously this was a silent no-op on LMDB). On LMDB, a valid `table` continues to scope the deletion to that table's history, unchanged. |
There was a problem hiding this comment.
delete_transaction_logs_before is dispatched as a background job, so the caller does not receive HTTP 400/404 from this request. The integration path in core PR #2064, “delete_transaction_logs_before with a table on RocksDB now returns an error instead of deleting the entire database's transaction log”, explicitly accepts HTTP 200, extracts job_id, and later observes job status ERROR; 400/404 are the internal error status codes. As written, clients may branch on the POST status and never poll the job. Please state that the request returns a job ID and the job ends in ERROR, and only describe the numeric codes if get_job exposes where clients can read them. A short initial-response/get_job example would make the contract unambiguous.
— KrAIs (Codex)
|
|
||
| <EngineBadge engines="RocksDB, LMDB" /> | ||
|
|
||
| On RocksDB (the default storage engine), deletion is database-wide: all tables in a database share one transaction log, so omit `table` and pass only `database` (or `schema`) and `timestamp`. |
There was a problem hiding this comment.
This correctly says RocksDB uses one database-wide transaction log, but the same page's introduction says both mechanisms are available at table level and its comparison table labels transaction-log storage as Clustering streams (per-table). Readers now get incompatible storage models on one page. Could you update that overview to distinguish logical per-table history/read filtering from RocksDB's physically shared database-wide log and purge scope?
— KrAIs (Codex)
|
|
||
| <VersionBadge type="changed" version="v4.5.0" /> — Storage reclamation: Harper automatically evicts older audit log entries when free storage drops below a configurable threshold | ||
|
|
||
| <VersionBadge type="changed" version="v5.2.0" /> — This operation requires `table`, but on the RocksDB storage engine (the default) history cannot be deleted for a single table because all tables in a database share one transaction log. On RocksDB this operation now always returns an error directing you to `delete_transaction_logs_before`; it remains usable on LMDB. |
There was a problem hiding this comment.
“Always returns an error directing you” is too broad. The companion core change resolves the table first, so a nonexistent table throws TABLE_NOT_FOUND/404 before reaching the RocksDB migration error; the new paragraph below also documents that case. Please qualify this as an existing RocksDB table, or say more generally that the operation is unsupported on RocksDB and direct valid-table callers to delete_transaction_logs_before.
— KrAIs (Codex)
|
|
||
| On RocksDB (the default storage engine), deletion is database-wide: all tables in a database share one transaction log, so omit `table` and pass only `database` (or `schema`) and `timestamp`. | ||
|
|
||
| <VersionBadge type="changed" version="v5.2.0" /> — On RocksDB, a request that includes `table` is now rejected with a `400` error; previously the `table` scope was silently ignored and the entire database's transaction log was purged. On either engine, a `table` that does not exist now returns a `404` (previously this was a silent no-op on LMDB). On LMDB, a valid `table` continues to scope the deletion to that table's history, unchanged. |
There was a problem hiding this comment.
The contribution guide requires a version badge to appear directly below the heading it annotates. Could you move this v5.2 change badge immediately below delete_transaction_logs_before (alongside or adjacent to the engine badge) and place the descriptive prose after it?
— KrAIs (Codex)
|
|
||
| <VersionBadge type="changed" version="v5.2.0" /> — On RocksDB, a request that includes `table` is now rejected with a `400` error; previously the `table` scope was silently ignored and the entire database's transaction log was purged. On either engine, a `table` that does not exist now returns a `404` (previously this was a silent no-op on LMDB). On LMDB, a valid `table` continues to scope the deletion to that table's history, unchanged. | ||
|
|
||
| On LMDB only, the optional `cleanup_deleted_records` (boolean) parameter additionally removes leftover tombstone entries for records deleted before the timestamp — a repair step for tombstones that normal audit log cleanup should already have removed. It is ignored on RocksDB. |
There was a problem hiding this comment.
The repository's content style requires type annotations in the form `<type>` (<modifier>). Could you describe this as `cleanup_deleted_records`: `boolean` (optional) (or equivalent) rather than putting (boolean) after the parameter name?
— KrAIs (Codex)
2 participants
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