perf(ledger): store commit sequence, not the receipt - #683
perf(ledger): store commit sequence, not the receipt#683ScriptedAlchemy wants to merge 1 commit into
Conversation
The writer idempotency ledger averages 1,486 bytes per row, which exceeds the 1,002-byte maxLocal of a 4 KiB index b-tree page, so every row spills into an overflow page of its own. Across four real stores that is 224,508 rows and 224,508 overflow pages holding ~1,005 bytes each: 1,004 MB of 3,228 MB, and 77% of one store. The 587-byte original_receipt_json column was the cause and was already redundant. StoreCommitReceiptV1 is deny_unknown_fields over seven fields and six were already columns; decode_row required every one of those equalities and failed closed otherwise, so the encoded receipt was never authority for anything but its commit_sequence. Replacing it with that integer keeps the receipt exactly reconstructible and drops the row under maxLocal. Verified byte-for-byte over all 224,508 real rows. The ledger shards sit outside the schema framework at user_version 0 and share their file with 200-odd unrelated tables, so the shape itself is the version: the presence of original_receipt_json is the old one. The upgrade runs in the writer's own transaction, which makes an interruption a rollback to a shape that re-triggers it, and a row whose commit sequence is not a positive integer aborts it with the same Corrupt class that reading the row already raised, rather than dropping a record the ledger must still recognise. Measured on a copy of a real 94,366-row store: the ledger falls from 442,458,112 to 96,649,216 bytes with zero overflow pages, the store from 1,604,157,440 to 1,239,306,240, and every receipt round-trips unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cc123c8dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| WHERE typeof(json_extract(original_receipt_json, '$.commit_sequence')) <> 'integer' | ||
| OR json_extract(original_receipt_json, '$.commit_sequence') <= 0 |
There was a problem hiding this comment.
When a legacy row contains syntactically valid JSON with a positive commit_sequence but an incomplete receipt such as {"commit_sequence":7}, or a full receipt whose operation, digest, binding, or timestamp disagrees with the duplicated columns, this preflight passes and the migration permanently drops the evidence of corruption. Before this commit, decode_row decoded the complete canonical StoreCommitReceiptV1 and checked every duplicated field; afterward it synthesizes a valid receipt from the columns and can return Replay/Conflict instead of failing closed. Validate and compare the complete legacy receipt before projecting it to the narrow table.
AGENTS.md reference: AGENTS.md:L126-L128
Useful? React with 👍 / 👎.
|
Post-merge review found two blockers in the integrated migration: it copies the full legacy table on the first foreground writer transaction, defeating bounded pruning, and it drops original_receipt_json after validating only commit_sequence rather than all legacy receipt invariants. The integration was reverted from PR #421 in the follow-up revert commit; this PR remains closed pending a bounded/background migration with full corruption validation. |
1 participant
What
td_runtime_writer_idempotency_v1replaces its 587-byteoriginal_receipt_jsoncolumn with a
commit_sequenceinteger, and gains a transactional in-placeupgrade so existing stores move to the narrow shape.
Why the receipt is reconstructible
This is the load-bearing claim, so it is argued twice — structurally and
empirically.
Structurally.
StoreCommitReceiptV1is#[serde(deny_unknown_fields)]overexactly seven fields. Six were already columns of this same row:
operation_idoperation_ididempotency.keyidempotency_keyidempotency.command_digestrequest_digestshard_idshard_jsonincarnationincarnationauthority_epochauthority_epochcommitted_atcommitted_at_microscommit_sequencedecode_rowalready required every one of those equalities and answeredLedgerError::Corruptotherwise, so the encoded receipt was never authority foranything except
commit_sequence. The reconstruction is that same check runbackwards. Four of the six — shard, incarnation, authority epoch, idempotency
key — are the primary key the row was matched on, so they are the caller's own
arguments by construction.
Empirically. Every real row on this machine was rebuilt byte-for-byte from
the six columns plus the sequence:
Why the row was spilling
maxLocalfor an index b-tree at 4 KiB pages is((4096-12)*64/255)-23 = 1002bytes. Measured payloads, and the projection after the change:
Every one of the 224,508 rows was over
maxLocalbefore and none is after. Thedbstatpage census showed overflow pages exactly 1:1 with rows in all fourstores (24,558 / 40,003 / 94,313 / 65,634), each holding ~1,005 bytes in a
4,096-byte page — 688 MB of pure slack.
Note the remaining headroom is ~100 bytes, and the largest remaining column is
transaction_scope_jsonat 456 bytes, which re-encodes the binding anddurability this row already carries. Normalising it (and
shard_json) is theobvious follow-up, but it is a separate change and this one has to land first.
Why the migration is safe
The ledger shards sit at
user_version = 0, outside the schema framework, andshare their file with ~207 unrelated tables. Claiming that file-global pragma
for the ledger would let a stamp disagree with the shape it describes, so
the shape is the version: the presence of
original_receipt_jsonis the oldone, read from
pragma_table_info, which is derived from the same schema recordthe queries compile against and therefore cannot drift from what is there.
existing transaction, and SQLite DDL is transactional. A crash rolls back to
the old table including its column set, so the next open re-detects the old
shape and retries. There is no state outside the transaction that could
disagree with it, so there is no half-migrated state to detect.
writers across processes. A racing process either waits for the exclusive lock
and observes the new shape, or is refused
SQLITE_BUSYhaving changed nothing.SELECTnamesoriginal_receipt_json, whichno longer exists, so the statement fails to prepare. The direction matters:
it is refused, never told "no such row", so it cannot mistake an unreadable
shape for an absent record and admit a duplicate. Pinned by
an_older_binary_is_refused_by_a_migrated_store_rather_than_misreading_it.This is forward-only, consistent with the framework's own stance that a store
at an unexpected shape "is refused at open".
the migration with the same
Corruptclass that reading that row alreadyraised, leaving the store byte-identical at the old shape.
typeofis checkedexplicitly because SQLite orders text above every integer, so a bare
> 0would have let
"not-a-number"through. Dropping such a row instead wouldsilently shrink the set of submissions the ledger recognises — the one outcome
that could admit a duplicate write.
store), so the
DROP/RENAMEswap has no dependents.Measured, on a copy of a real 94,366-row store
The exact statements the migration runs, applied to a copy of a live store:
Ledger 442,458,112 → 96,649,216 bytes (78.2%, 345,808,896 saved), overflow
pages 94,366 → 0, whole store down 22.7%, every receipt unchanged.
RED / GREEN
RED, with the migration neutered to an early
Ok(())(the "row-width fix withno migration mechanism" state):
That third failure is itself the shape-mismatch evidence: a mismatch is a hard
SQL refusal, never a silent miss.
GREEN:
The migration tests start from
LEGACY_IDEMPOTENCY_DDL, the pre-change tabledefinition reproduced verbatim, with receipts encoded by the same serializer
production used — a real pre-migration shape, not something the new code could
have written.
Duplicate-admission behaviour is covered directly:
a_migrated_duplicate_still_replays_instead_of_committing_againseeds a legacyrow, migrates, then asserts a same-key/same-digest retry still
Replays theoriginal receipt, a same-key/different-digest submission still
Conflicts, andthat neither added a row.
Full crate suite: 440 tests across 20 binaries, all passing. No assertion
anywhere is on elapsed time.
Checks
Per-crate only, as requested.
Relationship to the pruning rule
perf(ledger): prune superseded idempotency recordsis not on this base branch.The two are complementary and near-disjoint — prune deletes unreachable rows,
this narrows the rows that remain. On merge the only textual overlaps are two
adjacent lines in
commit.rs(prune inserts aprune_supersededcall directlyabove the
idempotency::insertcall whosereceipt_jsonargument this removes)and separate test functions in
tests.rs; the migration tests were deliberatelyplaced in
migrate.rsto keeptests.rsconflict-free. Nothing in this changetouches
prune.rs'sDELETE, which selects onshard_jsonandauthority_epochonly.Scope deviation
tests.rs::malformed_canonical_json_fails_closedandtests/runtime_actor/faults.rsboth corruptedoriginal_receipt_jsontoassert fail-closed behaviour. That column no longer exists, so both were
retargeted to
transaction_scope_json, which is still an encoded column, topreserve each test's intent.
commit.rsno longer encodes the receipt to JSON, so its now-unusedencode_jsonimport was dropped.transaction_scope_jsonandshard_jsonnormalisation (the further ~20%), and any change to
td_runtime_writer_checkpoint_v1, which holds one row per shard/incarnationand is not a size problem.
🤖 Generated with Claude Code