Repository navigation
Conversation
Every keyed write checked one 32 MiB ceiling on a batch's whole Arrow memory, which includes a managed Blob value's bytes, so a value of exactly 32 MiB never fit beside its row. KeyedBytes splits that count into payload (each managed value's logical length) and framing (the rest), each under its own 32 MiB ceiling, and every keyed check uses it: staging, keyed-write preparation, the update carry scan, branch merge buffering and proven-insert chunking, and the compatibility loader's pre-decode forecast. The halves sum to the old count. A payload refusal names its resource with 'Blob payload bytes' in place of 'bytes'. Two Lance guards pin what the Blob write plan relies on: a whole-row merge-update keeps the row's stable row id, and merge-insert refuses an external reference outside the dataset's bases.
Session::put_blob_at_as and clear_blob_at_as replace or clear one Blob value of an existing node or edge, addressed by exact id like a Blob read. A cell write is a Mutation-protocol write: it stages one upsert of the carried row and runs the Mutation tail, now split into commit_staged_mutation and publish_committed_mutation, which mutate uses too. The target's old payload is never read; the row's other cells are carried as an update carries them and share the 32 MiB payload allowance with the new value, which is bounded at 32 MiB inclusive before anything is captured. An optional precondition (an ETag set, or any existing value) is evaluated against the attempt's base and re-evaluated when a competing commit forces a re-prepare; a change of branch incarnation or accepted schema between attempts fails closed. The receipt's ETag is read from the detached commit before publication.
The compatibility loader's pre-decode forecast charged every `base64:` string as Blob payload, so a declared String whose text starts with `base64:` used the payload ceiling: a 24 MiB Blob beside such a 12 MiB String was refused at 33 MiB although the batch check admits it. The forecast now takes the type's Blob properties, as the Arrow check does: only a Blob property's `base64:` value is payload; every other value is framing.
# Conflicts: # crates/omnigraph-server/src/blob_transport.rs
…udget # Conflicts: # crates/omnigraph/src/table_store.rs
# Conflicts: # docs/dev/blob.md
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: hold the stack's merge until the minimal GQT regression requested on #903 is included. I found no new blocking code defect in step 3A. Reviewed cffa7918f3467a3a73a3f92ab487b2f0117ab501, including the changes above parent a4429d4508a64f80df1b1877dc251e724fbd5b86. This is a COMMENT review because the authenticated account is the author.
This PR lets an embedded caller replace or clear one Blob on an existing node or edge. The caller supplies an exact entity ID and can require a matching ETag. The engine carries the rest of the row, then publishes one graph commit. Missing IDs cannot become inserts. Empty bytes and null remain distinct. HTTP and CLI write commands remain separate work.
Correctness and design. The contract needs one accepted write snapshot, atomic graph visibility, preserved sibling values, and a receipt that identifies this write. The implementation addresses those obligations in their existing owners:
- The shared mutation tail validates and stages the row, commits it detached, and publishes through the existing manifest CAS. Ordinary mutations use the same tail. The write guards remain held through publication.
- Each retry captures fresh authority, checks the precondition again, and carries fresh sibling values. Changed branch or schema identity ends the retry. This covers both nodes and edges without a special storage path.
- The receipt comes from the detached version before publication. A later writer cannot change its ETag. Reads and writes share the same descriptor lookup and ETag calculation.
Tradeoffs and liability. This suits bounded replacement of an existing cell. It still rewrites the whole row. The new value and carried Blobs share 32 MiB, so a large sibling can prevent an otherwise small put. An external sibling needs policy admission and a readable source. The target's old payload is omitted from the carry, so replacing a stale external target does not read it.
Those costs follow the pinned substrate. In Lance 11.0.0, UpdateBuilder refuses Blob-v2 columns. The merge-insert strategy selector also excludes them from RewriteColumns. Its whole-row writer uses default external-reference restrictions. I checked the complete relevant upstream guides against this exact implementation.
The ETag also changes after another row in the same table changes. This is conservative and can reject a conditional write whose payload remains unchanged. Each put adds a descriptor read before publication. I did not measure throughput, contention, cloud request cost, or memory peaks. The 32 MiB payload allowance is not an RSS limit.
The PR adds public APIs, precondition/error/receipt contracts, and retry identity checks that need continued tests. It adds no durable format or second publication protocol. Sharing the mutation tail avoids duplicate validation and publication code. Five similar features can use that owner. The added liability is proportionate to the new interface, provided future surfaces retain this common path.
Tests and the existing merge prerequisite. GQT cannot call these new Session methods or assert their byte streams and ETags. The existing Rust owners are appropriate for that contract and its races. The parent commit has no cell-write API, so a compile failure there would not prove a regression.
The inherited payload/framing fix is different: GQT can express its boundary. The existing #903 review requests a compact permanent case, and the author's response proposes one after #927. This head still contains no such case. Keep that requirement before merging the stack. I did not duplicate it as a new inline defect here. No optional improvement is required by this review.
Local validation on the exact reviewed head passed:
- 13 focused tests for cell writes, payload limits, carried siblings, policy, retries, receipts, and shared Blob reads.
- All 28 put/clear cases in the default crash matrix, including fresh, read-only, and same-handle recovery where applicable.
- All 35 architecture guards, formatting, the 233-file documentation check, and the agent-link check.
The test commands used this environment and common prefix:
export RUSTUP_TOOLCHAIN=1.97.1
export CARGO_TARGET_DIR=/tmp/review-928/target
export RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable'
# Append the owner and filter from the table below:
cargo test --locked -p omnigraph-engine --features failpoints -j 3 --test OWNER FILTER -- --nocapture| Owner | Filter |
|---|---|
end_to_end |
blob_put_and_clear_replace_one_cell_by_exact_id (--exact), then blob_read_ |
writes |
blob_put_, then blob_write_under_deny_names_the_carried_reference_and_whole_row_writes_recover (--exact) |
writes |
exact_limit_blob_payload_fits_beside_its_row_and_one_more_byte_is_refused (--exact) |
failpoints |
blob_put |
policy_engine_chassis |
blob_put_and_clear_enforce_change_for_the_actor (--exact) |
detached_commit_matrix |
rfc_0067_failure_window_matrix (--exact), with OMNIGRAPH_MATRIX_WRITERS=BlobPut,BlobClear |
forbidden_apis |
No filter |
I also reran the identical temporary GQT boundary probe from the #903 review. It extends blob_update_carries_unassigned_blobs.gqt with one insert and a 32 MiB Blob parameter filled with byte 0x07. The assertion expects one affected node.
| Same regression | Commit | Result |
|---|---|---|
| Saved before-fix execution from the #903 review | b8b631ee8f813e557805baf33efe869011f48275 |
Failed at the added insert: keyed entity bytes for node:Doc, actual 33,556,272, limit 33,554,432 |
| Execution in this review | cffa7918f3467a3a73a3f92ab487b2f0117ab501 |
One ordinary filesystem execution passed in 8.19 seconds |
Both reports identify the same fixture digest, fb499d12646ca7dd0f58dddc909d8fa8a0ab508da8036511908c0bdbc0457892. The before result is retained evidence, not a new run. The head result executed the test, with no setup failure or skipped selected test. DST environments were unselected. The temporary file uses a 60-second timeout and 44.7 MB of literal text. It is evidence for the compact permanent case requested above, not a proposed fixture to commit.
Exact GQT commands, with the toolchain and flags above:
# Saved before-fix run at b8b631ee8f813e557805baf33efe869011f48275:
CARGO_TARGET_DIR=/tmp/review-903-followup/target cargo run -p omnigraph-gqt --locked --features omnigraph/failpoints --bin omnigraph-gqt -- /tmp/review-903-followup/blob_limit.gqt --target omnigraph-engine --storage local-filesystem --artifacts /tmp/review-903-followup/gqt-before-artifacts
# Current run at cffa7918f3467a3a73a3f92ab487b2f0117ab501:
CARGO_TARGET_DIR=/tmp/review-928/target cargo run -p omnigraph-gqt --locked --features omnigraph/failpoints --bin omnigraph-gqt -j 3 -- /tmp/review-903-followup/blob_limit.gqt --target omnigraph-engine --storage local-filesystem --artifacts /tmp/review-908/gqt-artifactsExact-commit workspace CI passed. Its log confirms execution of the new tests and crash matrix. GQT and the pinned DST suite also passed. RustFS and Azure integration jobs passed in the workspace run. These are CI results, separate from the local checks above.
All local checks used Rust 1.97.1, locked dependencies, and the isolated checkout. The checkout is clean. No source or test edits were needed, and no PR changes were pushed. Local cloud storage, DST, and performance checks were not run.
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: accept the embedded cell-write design at d2b3860a91656fe5aecc62016ae2bc0c52c85a92. Resolve the conflicts with current main and validate that result before merge. I found no new blocking code defect. This follows the review of cffa7918f3467a3a73a3f92ab487b2f0117ab501.
This PR lets an embedded caller replace or clear one Blob cell by exact node or edge ID. The entity must already exist. A caller can supply an ETag to prevent an overwrite after a competing change. Empty bytes remain distinct from null. HTTP and CLI writes remain outside this PR.
Each attempt captures one accepted write view. It carries the row's other fields, stages one upsert, and uses the shared mutation publisher. The result's ETag comes from the detached version before publication. A retry captures fresh authority and checks the precondition again. A changed schema or branch incarnation stops the retry. See attempt identity, row construction, and the shared mutation tail.
The general invariant is that changing one cell preserves its siblings and publishes one coherent graph state. The change enforces this through the existing row writer and publication contract. It does not add a special publication path. The merged schema work also handles null Blob descriptors without requiring a physical data file. That matters for fields added to existing fragments. See descriptor identity.
There is a concrete cost. Lance 11.0.0 refuses direct Blob-v2 updates and excludes them from RewriteColumns. One cell write therefore reads and rewrites surviving sibling Blobs. External siblings need policy approval and become managed bytes. The old target payload is neither read nor charged. This suits bounded document updates, but can amplify I/O for rows with large siblings. ETags also depend on the table version, so an unrelated row change can invalidate one. I made no throughput or peak-memory claim.
The design adds two public write methods, preconditions, result types, and bounded retry obligations. These are lasting maintenance costs. Sharing validation, staging, and publication avoids a second commit protocol and reduces the greater liability of divergent writers. Five similar API additions should continue to use these owners. The row rewrite remains a substrate constraint, not a new storage abstraction. Deployment and writer-admission limits remain the repository's existing limits.
The earlier GQT hold needs an update. #933 is now on main as 1beab85547c218869e787a0b2aff04ed221346b9. It supplies small permanent cases such as write_max_bytes.gqt and the carried-Blob case. It also replaces fixed accounting with a captured WriteBudget. GitHub currently reports conflicts. When resolving them, use that shared budget for cell-write admission and sibling materialization, and keep those GQT cases. Maintaining the older accounting beside main's accounting would add avoidable liability. This integration is required work, not a defect reproduced in the reviewed head. The merged result does not yet exist for review.
Local validation on this head passed:
- 13 focused cell-write, Blob-read, boundary, policy, and retry tests.
- All 28 selected
BlobPut/BlobClearcrash-matrix cells and 35 forbidden-API guards. - A temporary extension of the existing
blob_put_and_clear_replace_one_cell_by_exact_idowner. It adds nullable Blob fields to existing node and edge tables, then checks no-op clear, put, sibling preservation, and clear. It passed. I restored the original file. - The minimal boundary GQT described below, formatting, documentation, and agent-link checks.
The cell API, ETag receipt, and payload-byte checks need the Rust owner. GQT has no cell put/clear operation and cannot project Blob bytes. Adding a superficially related query case would not cover that contract. The inherited payload-accounting bug is expressible in GQT.
The same minimal GQT fixture was used for its before/after proof: one Doc type, slug and nullable content, one exact-32-MiB insert, then a query that checks the key. SHA256: c3d5b93e8ddde0057bc144b16722ffec426160b42a8ed255994b1536050f344a.
- Before:
7a3bddf544640135a39cdec7952872b3ce0e3918, the retained local run before #933 changed main's accounting. The insert failed with actual row bytes33555360, limit33554432. It compiled and executed the case. This is evidence from the earlier review, not a new run today. - After: this exact PR head,
d2b3860a91656fe5aecc62016ae2bc0c52c85a92, which includes #903's accounting change. The same case passed locally in 8.15 seconds. The large value is necessary to reach the old fixed ceiling. Main now has small permanent boundary fixtures.
Commands used for that comparison, with separate output directories:
env RUSTUP_TOOLCHAIN=1.97.1 CARGO_TARGET_DIR=/tmp/review-928/target \
RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' \
cargo build --locked -p omnigraph-gqt --features omnigraph/failpoints \
--bin omnigraph-gqt -j 3
/tmp/review-928/target/debug/omnigraph-gqt /tmp/review-933/default_boundary.gqt \
--target omnigraph-engine --storage local-filesystem \
--artifacts /tmp/review-908-merge/default-afterThe before run used /tmp/review-933/default-before as its artifact directory. Focused Rust commands used the same environment and cargo test --locked -p omnigraph-engine --features failpoints -j 3 --test OWNER FILTER -- --nocapture. Owners were end_to_end, writes, policy_engine_chassis, and failpoints. The matrix used --test detached_commit_matrix rfc_0067_failure_window_matrix -- --exact --nocapture with OMNIGRAPH_MATRIX_WRITERS=BlobPut,BlobClear. The guard command used --test forbidden_apis without a filter.
CI provides separate evidence. The workspace job passed 4,080 tests and skipped 17. The ordinary GQT job passed 312 cases, ignored one, and passed its separately selected case. Their checkout was 357ebc6f59df5eae9c26239b4649f0a3014cc540. Its tree equals this head's tree, 98b940980c1a6a4a254a29fa87606207d67c6037. These runs do not validate a merge with today's main.
I reused the complete relevant upstream documentation and checked the pinned Lance 11.0.0 implementation. Local execution used macOS and filesystem storage. I did not rerun cloud suites or measure production concurrency, throughput, or RSS. All temporary source edits are restored. No new inline defect is asserted. The remaining merge integration must be checked on its new exact head.
Main's write_max_bytes setting landed the payload/row accounting split this branch inherited from its accounting prerequisite, so the merge keeps main's accounting and drops the prerequisite's. The Blob put now checks its value against the session's write_max_bytes and stages under that operation's WriteBudget. The prerequisite's two Lance surface guards, which pin facts the put relies on, come along unchanged.
RFC 0033 step 3A: the engine half of the Blob write API. An embedded
Sessioncan now replace or clear one Blob value of an existing node or edge, addressed by exact id like a Blob read.The plan this implements is the RFC 0033 amendment in #900. Step 3-pre, the payload/row accounting split, landed on main with the
write_max_bytessetting (#933), so this branch now builds on main's accounting directly. #903's own accounting is not merged here; its two Lance surface guards, which pin facts the put relies on, moved into this PR.What it adds
Session::put_blob_at_as(branch, cell, bytes, precondition, actor)stores managed bytes and returnsBlobWriteOutcome::Managed { length, etag, commit }.Session::clear_blob_at_as(branch, cell, precondition, actor)sets a nullable cell to null. It returnsNull { commit: None }when the cell is already null and no precondition is given.BlobPrecondition::{Tags, AnyExisting}and the newOmniError::BlobWritePreconditionFailed { current_etag }. The server maps it to its existing 412 Blob precondition response, so 3B only needs routing.BlobEtag::from_tag, so a caller can pass back a tag it received.Design
Not a new writer kind. A cell write is a Mutation-protocol write. Each attempt:
WriteTxn;PendingMode::Upsertrow;The tail is now split into
commit_staged_mutation, which does validation, staging andcommit_all's detached commit, andpublish_committed_mutation, the protocol's one publisher call.mutateuses the same two halves, so the publication path is unchanged.forbidden_apis.rsregistersexec/blob_write.rsunderMUTATION_V9, and no new durable call site appears.Whole-row replacement. Lance 11 has no single-cell Blob write:
UpdateBuilderandRewriteColumnsrefuse Blob-v2 columns. So the row is rebuilt:LargeBinarychild adopts the caller'sBytesbuffer without a copy.scan_with_pending_materialized_blobsof the exact id, with the target omitted. The old payload of the target is never read or charged.updatecarries them. A stored external sibling needs the external Blob policy; otherwise the write fails withStoredExternalBlobDeniednaming it.write_max_bytes, main'sWriteBudget) with the new value.Bound. A put larger than the session's
write_max_bytes(32 MiB by default) is refused before anything is captured, with resourceBlob write payload bytes. A value of exactly the allowance is accepted. The put captures itsWriteBudgetonce per operation from the session's settings, as a mutation does, so a lowered setting lowers both the put's bound and the payload allowance its carried siblings share.Preconditions and retries.
read_blob_atcomputes it.ReadSetChangedre-prepares the whole attempt, up to the insert-only bound of 32. Each new attempt re-reads the cell, re-evaluates the precondition and re-carries the row from the fresh base. A stale ETag therefore fails instead of overwriting.Receipt. The returned ETag is read from the detached version
commit_allproduced, before the manifest CAS. Nothing after publication reads storage, so a later write cannot change it.Deviations from the RFC text
current_etagisOption<String>, notOption<BlobEtag>: the error lives inomnigraph-core, which cannot name the engine'sBlobEtag. The string is the same quoted tag.Tests
end_to_end.rs::blob_put_and_clear_replace_one_cell_by_exact_idAnyExisting. Clear with a tag. A no-op clear returns no commit.AnyExistingon null fails withcurrent_etag: None. A missing id isNotFound.writes.rsBlob write payload bytes; withwrite_max_bytes = 4096, an exact 4,096-byte put is accepted, 4,097 bytes are refused, and a 2,000-byte put beside a 3,000-byte sibling is refused asdecoded blob input bytes per operation(5,000 of 4,096), all without a graph effect. The put never probes or reads its stale external target. The managed sibling stays byte-identical, and an external sibling becomes managed. Under deny, a put or clear names the carried reference on a node and an edge; a merge load recovers both, and a.gqupdate recovers the node.policy_engine_chassis.rschangefor the actor; a write with no actor is refused.detached_commit_matrix.rsBlobPutandBlobClearwriters in every window × fault × recovery actor. The oracle adds the cell value: the new value exactly when acknowledged, read from a fresh handle and from the writer's own.failpoints.rsblob_write.rslance_surface_guards.rs(from #903)merge_insert_refuses_an_external_blob_outside_the_dataset_bases: Lance's merge-insert cannot store an outside-base external reference, which is why a carried external sibling is read and stored as managed bytes (RFC 0033 §4.3; red when Phase 4's reference keep opens).filtered_scan_tolerates_merge_update_row_id_overlapalso pins that a whole-row merge-update keeps the row's stable row id, which the receipt's ETag reads back.Not in this PR
PUT/DELETEon/blob.blob put/blob clear.The docs say the CLI and server do not offer these writes yet.
RFC 0033 records that step 3-pre landed with
write_max_bytes, and that the put honors the setting (decision log, §4.3, §10, §13).