Conversation
The plan, staging, WAL, and Data Planes already carried EdgePutBatch and EdgeDeleteBatch; the statement layer above them was missing. Every bulk loader paid one statement per edge. GRAPH INSERT EDGES takes a property-less VALUES list of (src, dst, label) triples, capped at 1000 edges per statement with the cap named in the error. Each edge reuses the single-edge path, so surrogate assignment, RLS resolution, and dual-home routing stay identical to the singular form, and the edges bucket by home vShard so a single-home batch is one apply burst per home. build_static_tx_class derives participant homes and lock identity from batch edge plans for the Calvin path. The statement is property-less by design: BatchEdge carries no property object, so per-edge PROPERTIES stays on the single-edge form, and an edge whose write policy rewrites the property image keeps the single-edge dispatch.
A batch delete under a FOR WRITE owner policy evaluates the policy per edge: the conforming edge is deleted, the denied edge survives, and the statement reports an error. A property-less batch insert under an owner policy is refused with nothing landing, matching the batch-edge decision in the RLS injection pass.
e722edb to
26afed0
Compare
There was a problem hiding this comment.
This PR requires corrections at the shared parsing, routing, transaction, and error boundaries. The batch statement and per-edge policy handling belong together, but the current execution paths do not preserve the complete contract.
- Blockers: unrelated-home writes, partial durable effects, and malformed tuples creating unintended edges.
- Required corrections: single-participant routing, applied counts, typed errors, complete deletion batching, bounded parsing, and decisive security coverage.
- Hygiene: remove stale dispatch comments and follow the runtime JSON convention.
- Integration: this PR overlaps ongoing graph parser, property-encoding, and shared Calvin write-key changes. Wait until those changes merge, then submit a focused PR against the resulting interfaces. Preserve the shared implementation instead of restoring older extraction logic.
The inline requests require complete corrections at the owning layers. Temporary loops, swallowed errors, preliminary checks, and compensating writes do not satisfy them.
| tenant_id, | ||
| vshard_id: VShardId::from_key(collection.as_bytes()), | ||
| database_id, | ||
| plan: PhysicalPlan::Graph(GraphOp::EdgePutBatch { edges: batch }), |
There was a problem hiding this comment.
[P1] Project each batch onto its actual participant homes
You submit one unsliced EdgePutBatch to every endpoint home. local_calvin_plans retains the entire plan whenever any edge homes locally. execute_edge_put_batch then applies every edge. With A→B and C→D on disjoint homes, each participant stores both edges. A later singular delete reaches only the deleted edge's actual homes, leaving stray copies elsewhere.
Correct the shared participant-projection boundary before staging and execution. Preserve one transaction while each participant receives only its owned edges. Add cluster coverage for disjoint homes and subsequent deletion.
| for (vshard, group) in buckets { | ||
| let plan = PhysicalPlan::Graph(GraphOp::EdgePutBatch { edges: group }); | ||
| let response = | ||
| crate::control::server::sync::raft_dispatch::dispatch_trusted_internal_sync_response( |
There was a problem hiding this comment.
[P1] Commit or abort the complete batch statement
You dispatch and commit source-home buckets independently. A valid early bucket persists before a later bucket rejects a dangling endpoint. The statement then returns an error with durable partial effects. delete_edges repeats this through individual deletes. Its RLS test explicitly requires the first deletion to survive the second edge's refusal.
Route the complete statement through shared transaction admission, staging, and commit/abort. Preserve atomicity across edge state, WAL, and events. Update the RLS test to require both edges unchanged after rejection. A preliminary check or compensating delete loop does not protect against concurrent changes or durable side effects.
| txn_id: None, | ||
| }); | ||
| } | ||
| let tx_class = build_static_tx_class(&tasks, tenant_id, &[]) |
There was a problem hiding this comment.
[P2] Select execution from the actual participant set
You always call the strict multi-vShard builder whenever Calvin exists. That builder rejects a write set containing only one participant. VALUES ('a','a','knows') therefore fails on a clustered node, despite being a valid edge. Distinct endpoints that hash to one vShard fail the same way.
Use the shared single-participant route when the resolved batch has one home. Preserve admission and statement atomicity. Add coverage for self-loops and distinct co-resident endpoints.
| }); | ||
| } | ||
| let edges = fields | ||
| .chunks(3) |
There was a problem hiding this comment.
[P1] Preserve tuple structure before constructing edge identities
The tokenizer discards parentheses and commas. Grouping its flattened fields into threes cannot check the original tuples. VALUES ('a','b'), ('c','d','L','M') passes and creates (a,b,c) and (d,L,M). The parser also accepts missing delimiters and an unterminated final quoted value.
Correct the structural parser or tokenizer boundary. Check clause order, tuple arity, delimiters, quote closure, and trailing input before constructing edges. Reject malformed input before dispatch. Add negative cases for both insert and delete.
| } | ||
| Ok(vec![DdlResult::Status { | ||
| command: "DELETE EDGES".to_string(), | ||
| rows_affected: Some(count), |
There was a problem hiding this comment.
[P2] Return the applied logical delete count
You discard every delete_edge result and return the input length. An absent edge reports one deletion. Repeating one existing tuple twice reports two deletions instead of one. The singular path already reads the actual affected count.
Carry logical affected counts through the batch executor and shared response decoder. Count each removed edge once across its endpoint homes. Cover absent edges, duplicate tuples, and transactional staging. Also reject a missing Calvin applied response instead of accepting None as success.
| "GRAPH NEIGHBORS IN '{collection}' OF '{EDGE_SRC}' DIRECTION out LABEL '{label}'" | ||
| )) | ||
| .await | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
[P2] Make security probes distinguish rejection from broken reads
This helper converts query errors into absence. Its JSON decoding also converts malformed responses into absence. The insert probe accepts any write error, then uses this helper to assert that no edge exists. An unrelated dispatch error followed by broken reads therefore passes without proving RLS enforcement.
Preserve the typed database error and assert the expected authorization SQLSTATE. Require successful reads and valid response decoding. Use sonic_rs for runtime JSON decoding. Add explicit denied-GRANT coverage for both batch operations. Granting a broad role to an RLS test user does not test GRANT enforcement.
| .await | ||
| { | ||
| Ok(rows) => rows.join(""), | ||
| Err(_) => String::new(), |
There was a problem hiding this comment.
[P2] Check every deleted edge through successful reads
You convert every traversal error into an empty result. The assertion also passes when only one link disappears and the other edge survives. Breaking the chain does not prove every requested edge was deleted.
Read each seeded edge independently after deletion. Require successful decoding and assert absence for both identities. An unrelated query error must fail the probe.
| ) -> Result<Vec<DdlResult>, DdlError> { | ||
| validate_edge_batch(&collection, &edges, "GRAPH DELETE EDGES")?; | ||
| let count = edges.len() as u64; | ||
| for tuple in edges { |
There was a problem hiding this comment.
[P2] Complete deletion batching through the existing execution layers
This loop still performs one full delete_edge path per tuple. A 1,000-edge statement therefore retains 1,000 dispatch or transaction round trips. The parser batches SQL while execution retains the original per-edge cost.
Integrate batch deletion through shared planning, authorization, write resolution, staging, and execution. Extend the batch representation to carry required policy data. Preserve per-edge authorization and whole-statement atomicity. Do not ship the loop as a temporary substitute for that integration.
| let mut fields: Vec<String> = Vec::new(); | ||
| for t in &toks[values_pos + 1..] { | ||
| match t { | ||
| Tok::Quoted(s) => fields.push(s.clone().into_owned()), |
There was a problem hiding this comment.
[P2] Enforce the batch bound before copying the complete input
You copy every field into fields before checking the 1,000-edge limit. Oversized input consumes allocation and parsing work proportional to the full request before rejection. The tokenizer also materializes the complete token vector first.
Enforce limits while structurally parsing tuples, before collecting their owned strings. Keep a shared bound at the typed-plan admission boundary for callers that bypass SQL. Avoid duplicate full-input collections.
| /// edges and costs one round trip; each edge then reuses the single-edge path, | ||
| /// so surrogate assignment, RLS resolution, dual-home routing, and Calvin | ||
| /// semantics stay identical to the singular form. | ||
| /// |
There was a problem hiding this comment.
[P3] Describe the implemented dispatch behavior
This comment says every edge reuses the singular path and batching remains a follow-up. The function already constructs EdgePutBatch outside explicit transactions. The module comment repeats the same inaccurate claim.
Describe the actual transaction and batch paths. Remove the stale future-work narrative and file-splitting history. Keep comments focused on invariants and required behavior.
What
GRAPH INSERT EDGESgives a bulk loader one statement for many edges. The plan, staging, WAL, and Data Planes already carriedEdgePutBatch; the statement layer above them was missing, so every loader paid one statement per edge.The batch form takes a property-less
VALUESlist of(src, dst, label)triples, capped at 1000 edges per statement with the cap named in the error. Each edge reuses the single-edge path, so surrogate assignment, RLS resolution, and dual-home routing stay identical to the singular form. The edges bucket by home vShard, so a single-home batch is one apply burst per home.build_static_tx_classderives participant homes and lock identity from batch edge plans for the Calvin path.This also carries the RLS and GRANT probes, which #373 was consolidated into by the maintainer because the batch statement is their prerequisite.
Notes
BatchEdgecarries no property object, so per-edgePROPERTIESstays on the single-edge form. An edge whose write policy rewrites the property image therefore keeps the single-edge dispatch.GRAPH DELETE EDGESparses and routes as a batch, but the handler loops the single-edge path, so it wins at the parser and not at the executor.EdgeDeleteBatchexists in the data plane and no handler calls it. Called out here rather than left implied by the title.Evidence
Every test below fails on
mainwithout this change. Proof: the new test files were copied onto a cleanorigin/main(bd8da7dc2) worktree and run there first.cargo nextest run -p nodedb --test wire -E 'test(~graph_dsl_batch_edges)'on a cleanmain: FAIL, 3 tests (graph_insert_edges_batch_round_trips,graph_delete_edges_batch_removes_every_edge,batch_over_the_cap_is_rejected_and_names_the_cap).graph_dsl_batch_edges3/3 pass,graph_batch_rls3/3 pass.graph_dsl44/44,engine_surface_graph14/14,graph_timeseries_rls_probe7/7,pgwire_show_dispatch19/19.nodedb-sql: 1043/1043 pass.cargo check -p nodedb,cargo fmt --all -- --check, and a lib clippy run are clean.graph_delete_edges_batch_removes_every_edgefails; restored, it passes.What CI does not cover locally
Closes #369
Refs #373