From 3899eb173ab97b73343c540aec85159e5ea32f6f Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Sat, 19 Sep 2026 05:58:35 +0800 Subject: [PATCH 01/29] fix(resp): report row counts on KV, CRDT, and vector writes KV insert/update, upsert, ON CONFLICT resolution (both the direct and staged-transaction paths), CRDT document writes, and vector-primary upserts answered with a bare OK response instead of a Postgres command tag carrying an affected-row count. A bare tag parses to 0 in tokio_postgres, so a driver reads a successful write as having affected no rows. Add response_affected_with_op alongside the existing response_affected helper so upsert paths can report INSERT 0 n or UPDATE n depending on whether the write inserted or overwrote a key, and route the KV conflict-resolution handlers through it instead of hand-encoding the payload with response_codec. Extend the CommandComplete tag conformance tests to cover multi-row inserts, ON CONFLICT DO NOTHING, staged transactions, and every engine (document, strict document, KV, timeseries, spatial, vector-primary, CRDT, array) so each write path is pinned to the tag contract. --- .../src/data/executor/core_loop/response.rs | 17 + .../executor/handlers/control/crdt_doc.rs | 4 +- .../executor/handlers/kv/crud/write_basic.rs | 4 +- .../executor/handlers/kv/crud/write_upsert.rs | 7 +- .../stage_write/stage_kv_conflict.rs | 10 +- .../transaction/stage_write/stage_upsert.rs | 10 +- .../data/executor/handlers/vector_upsert.rs | 2 +- .../cases/command_complete_tag_conformance.rs | 444 +++++++++++++++++- 8 files changed, 451 insertions(+), 47 deletions(-) diff --git a/nodedb/src/data/executor/core_loop/response.rs b/nodedb/src/data/executor/core_loop/response.rs index 8a5e550e0..e1d50b6c7 100644 --- a/nodedb/src/data/executor/core_loop/response.rs +++ b/nodedb/src/data/executor/core_loop/response.rs @@ -82,6 +82,23 @@ impl CoreLoop { self.response_with_payload(task, payload) } + /// Build the response for a write whose affected count is fixed but whose + /// command verb is decided by the handler: `op` is `"insert"` or + /// `"update"`. Read by `extract_kv_conflict_op` on the Control Plane to + /// render `INSERT 0 n` or `UPDATE n`. + pub(in crate::data::executor) fn response_affected_with_op( + &self, + task: &ExecutionTask, + affected: u64, + op: &str, + ) -> Response { + let mut payload = Vec::with_capacity(32); + nodedb_query::msgpack_scan::write_map_header(&mut payload, 2); + nodedb_query::msgpack_scan::write_kv_i64(&mut payload, "affected", affected as i64); + nodedb_query::msgpack_scan::write_kv_str(&mut payload, "op", op); + self.response_with_payload(task, payload) + } + pub(in crate::data::executor) fn response_partial( &self, task: &ExecutionTask, diff --git a/nodedb/src/data/executor/handlers/control/crdt_doc.rs b/nodedb/src/data/executor/handlers/control/crdt_doc.rs index 16445425d..983094778 100644 --- a/nodedb/src/data/executor/handlers/control/crdt_doc.rs +++ b/nodedb/src/data/executor/handlers/control/crdt_doc.rs @@ -146,7 +146,7 @@ impl CoreLoop { } } } else { - self.response_ok(task) + self.response_affected(task, 1) } } else if let Some(spec) = returning { match returning_rows::build_rows_payload(spec, rls_filters, &[]) { @@ -161,7 +161,7 @@ impl CoreLoop { } } } else { - self.response_ok(task) + self.response_affected(task, 1) }; self.checkpoint_coordinator.mark_dirty("crdt", 1); response diff --git a/nodedb/src/data/executor/handlers/kv/crud/write_basic.rs b/nodedb/src/data/executor/handlers/kv/crud/write_basic.rs index 39594631c..4d0059475 100644 --- a/nodedb/src/data/executor/handlers/kv/crud/write_basic.rs +++ b/nodedb/src/data/executor/handlers/kv/crud/write_basic.rs @@ -80,7 +80,7 @@ impl CoreLoop { // stored post-image, not an echo of the request. return self.kv_stored_returning_response(task, spec, rls_filters, &[(key, value)]); } - self.response_ok(task) + self.response_affected(task, 1) } /// SQL `INSERT` semantics: write only if key doesn't already exist. @@ -167,7 +167,7 @@ impl CoreLoop { if let Some(spec) = returning { return self.kv_stored_returning_response(task, spec, rls_filters, &[(key, value)]); } - self.response_ok(task) + self.response_affected(task, 1) } /// SQL `INSERT ... ON CONFLICT DO NOTHING` semantics: write if absent, diff --git a/nodedb/src/data/executor/handlers/kv/crud/write_upsert.rs b/nodedb/src/data/executor/handlers/kv/crud/write_upsert.rs index 11ba0a53c..562522a5b 100644 --- a/nodedb/src/data/executor/handlers/kv/crud/write_upsert.rs +++ b/nodedb/src/data/executor/handlers/kv/crud/write_upsert.rs @@ -155,6 +155,11 @@ impl CoreLoop { &[(key, stored_bytes.as_slice())], ); } - self.response_ok(task) + let op_str = if existing_bytes.is_some() { + "update" + } else { + "insert" + }; + self.response_affected_with_op(task, 1, op_str) } } diff --git a/nodedb/src/data/executor/handlers/transaction/stage_write/stage_kv_conflict.rs b/nodedb/src/data/executor/handlers/transaction/stage_write/stage_kv_conflict.rs index 9883e8ed1..1cd7cbb1d 100644 --- a/nodedb/src/data/executor/handlers/transaction/stage_write/stage_kv_conflict.rs +++ b/nodedb/src/data/executor/handlers/transaction/stage_write/stage_kv_conflict.rs @@ -19,7 +19,6 @@ use nodedb_physical::physical_plan::UpdateValue; use super::context::StageCtx; use crate::bridge::envelope::{ErrorCode, Response}; use crate::data::executor::core_loop::CoreLoop; -use crate::data::executor::response_codec; impl CoreLoop { // ── InsertOnConflictUpdate: resolve current, merge, tag by outcome ────── @@ -111,13 +110,6 @@ impl CoreLoop { return self.response_error(ctx.task, e); } - let payload = match response_codec::encode_json_as_msgpack(&serde_json::json!({ - "affected": 1, - "op": op, - })) { - Ok(p) => p, - Err(e) => return self.response_error(ctx.task, e), - }; - self.response_with_payload(ctx.task, payload) + self.response_affected_with_op(ctx.task, 1, op) } } diff --git a/nodedb/src/data/executor/handlers/transaction/stage_write/stage_upsert.rs b/nodedb/src/data/executor/handlers/transaction/stage_write/stage_upsert.rs index 1d100b7d3..a402bdba1 100644 --- a/nodedb/src/data/executor/handlers/transaction/stage_write/stage_upsert.rs +++ b/nodedb/src/data/executor/handlers/transaction/stage_write/stage_upsert.rs @@ -20,7 +20,6 @@ use crate::bridge::envelope::Response; use crate::data::executor::core_loop::CoreLoop; use crate::data::executor::handlers::transaction::overlay::Staged; use crate::data::executor::handlers::upsert::{apply_on_conflict_updates, merge_values}; -use crate::data::executor::response_codec; use crate::data::executor::strict_format; use crate::types::TenantId; @@ -78,14 +77,7 @@ impl CoreLoop { return self.response_error(ctx.task, e); } - let payload = match response_codec::encode_json_as_msgpack(&serde_json::json!({ - "affected": 1, - "op": op, - })) { - Ok(p) => p, - Err(e) => return self.response_error(ctx.task, e), - }; - self.response_with_payload(ctx.task, payload) + self.response_affected_with_op(ctx.task, 1, op) } /// Resolve the current stored body for `ctx` under BASE ∪ OVERLAY: a diff --git a/nodedb/src/data/executor/handlers/vector_upsert.rs b/nodedb/src/data/executor/handlers/vector_upsert.rs index a201e59e8..c63738fc7 100644 --- a/nodedb/src/data/executor/handlers/vector_upsert.rs +++ b/nodedb/src/data/executor/handlers/vector_upsert.rs @@ -289,7 +289,7 @@ impl CoreLoop { &sidecar, ); } - self.response_ok(task) + self.response_affected(task, 1) } } diff --git a/nodedb/tests/wire/cases/command_complete_tag_conformance.rs b/nodedb/tests/wire/cases/command_complete_tag_conformance.rs index 36eb44ff8..a3cdfc502 100644 --- a/nodedb/tests/wire/cases/command_complete_tag_conformance.rs +++ b/nodedb/tests/wire/cases/command_complete_tag_conformance.rs @@ -1,48 +1,99 @@ // SPDX-License-Identifier: BUSL-1.1 -//! Pins two `CommandComplete` tag contracts: the object-literal -//! `INSERT INTO t { ... }` path must not omit its affected-row count -//! entirely (a bare `INSERT` tag real `psql` cannot parse), and `TRUNCATE -//! TABLE` must not append a row count Postgres's tag never carries. +//! Pins the `CommandComplete` tag contract every DML statement must honour: +//! ONE statement answers with ONE tag, the tag is a Postgres command tag +//! (`INSERT 0 n` / `UPDATE n` / `DELETE n`), and `n` is the number of rows the +//! statement affected — whatever engine the collection runs on and however +//! many Data-Plane tasks the statement planned to. +//! +//! Drivers turn the tag into `rowcount` / `rows_affected`, and ORMs turn that +//! into "did my write land?" — batch-load accounting and optimistic-locking +//! checks both read it. A statement that answers with several tags makes the +//! driver report the LAST one (or the first, depending on the driver); a bare +//! `OK` tag has no count to parse at all. //! //! `tokio_postgres::SimpleQueryMessage::CommandComplete` exposes only a //! `u64`, derived by `tokio_postgres::query::extract_row_affected` as //! `tag.rsplit(' ').next()` — the LAST whitespace-separated token, parsed as -//! an integer (falling back to `0` if that fails). This makes the two cases +//! an integer (falling back to `0` if that fails). This makes the cases //! below observable: //! -//! - a bare tag (no trailing integer) always parses to `0` via the fallback; +//! - a bare tag (no trailing integer, e.g. `OK`) always parses to `0`; //! - a tag with a trailing integer parses to that integer regardless of how -//! many tokens precede it. +//! many tokens precede it; +//! - every `CommandComplete` in a simple-query response is surfaced, so a +//! statement that answers with several tags is countable; +//! - `Client::execute` (extended query) reads to `ReadyForQuery` and returns +//! the LAST tag's count, so a per-row tag sequence reports the final row's +//! `1` for the whole statement. //! -//! It does NOT make the `INSERT` OID fix observable: `INSERT 1` (malformed, -//! oid omitted) and `INSERT 0 1` (correct) both end in `1`, so -//! `extract_row_affected` returns `1` either way. Distinguishing those two -//! requires reading the raw tag string, which `tokio_postgres`'s public API -//! never surfaces (only the derived count crosses the crate boundary) — so -//! that half of the fix has no assertion here that could fail against the -//! pre-fix code. The bare-tag and TRUNCATE-count regressions below are -//! within the driver's reach and are exercised directly. +//! It does NOT make the `INSERT` OID observable: `INSERT 1` (malformed, oid +//! omitted) and `INSERT 0 1` (correct) both end in `1`. Distinguishing those +//! requires the raw tag string, which `tokio_postgres` never surfaces. use crate::harness::TestServer; use tokio_postgres::SimpleQueryMessage; -/// The row count carried by the first `CommandComplete` in `sql`'s response. -async fn affected(server: &TestServer, sql: &str) -> u64 { +/// Every `CommandComplete` count in `sql`'s simple-query response, in wire +/// order, plus the number of `Row` messages that arrived alongside them. +async fn command_tags(server: &TestServer, sql: &str) -> (Vec, usize) { let messages = server .client .simple_query(sql) .await .unwrap_or_else(|e| panic!("run {sql}: {e:?}")); - messages - .into_iter() - .find_map(|m| match m { - SimpleQueryMessage::CommandComplete(n) => Some(n), - _ => None, - }) + let mut tags = Vec::new(); + let mut rows = 0; + for m in messages { + match m { + SimpleQueryMessage::CommandComplete(n) => tags.push(n), + SimpleQueryMessage::Row(_) => rows += 1, + _ => {} + } + } + (tags, rows) +} + +/// The row count carried by the first `CommandComplete` in `sql`'s response. +async fn affected(server: &TestServer, sql: &str) -> u64 { + let (tags, _) = command_tags(server, sql).await; + tags.first() + .copied() .unwrap_or_else(|| panic!("statement reported no command tag: {sql}")) } +/// Assert that ONE DML statement answered with exactly one command tag whose +/// count is `expected`, and that no row data rode along with it. +async fn assert_single_tag(server: &TestServer, sql: &str, expected: u64) { + let (tags, rows) = command_tags(server, sql).await; + assert_eq!( + tags.len(), + 1, + "one statement must answer with exactly one CommandComplete, got {tags:?} for: {sql}" + ); + assert_eq!( + tags[0], expected, + "command tag must carry the statement's affected-row count for: {sql}" + ); + assert_eq!( + rows, 0, + "a DML statement without RETURNING must not answer with row data for: {sql}" + ); +} + +/// Number of rows a `SELECT count(*)` reports — the observable state the +/// affected count must agree with. +async fn live_rows(server: &TestServer, sql: &str) -> u64 { + let rows = server + .query_text(sql) + .await + .unwrap_or_else(|e| panic!("count query should succeed: {sql}: {e}")); + rows.first() + .unwrap_or_else(|| panic!("count query returned no row: {sql}")) + .parse() + .unwrap_or_else(|e| panic!("count query returned a non-integer: {sql}: {e}")) +} + /// `INSERT INTO t { ... }` (the object-literal insert path, /// distinct from `INSERT ... VALUES`) must not return `DdlResult::Status { /// rows_affected: None, .. }`, which pgwire renders as a bare `INSERT` tag. @@ -102,3 +153,350 @@ async fn truncate_table_reports_no_row_count() { parse must read 0 — not the number of rows actually removed" ); } + +/// A multi-row `INSERT ... VALUES (...), (...), (...)` on a schemaless +/// document collection is one statement and answers with one `INSERT 0 3`, +/// not one `INSERT 0 1` per value tuple. The planner lowers the statement to +/// one task per row; that fan-out is an execution detail the wire must not +/// expose. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn multi_row_insert_reports_one_tag_with_row_count() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION tag_multi_doc (id INT PRIMARY KEY, v TEXT)") + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + + assert_single_tag( + &server, + "INSERT INTO tag_multi_doc (id, v) VALUES (1, 'a'), (2, 'b'), (3, 'c')", + 3, + ) + .await; + assert_eq!( + live_rows(&server, "SELECT count(*) FROM tag_multi_doc").await, + 3, + "all three rows must have landed" + ); +} + +/// The extended-query path (`Parse`/`Bind`/`Execute`, what psycopg2 and every +/// ORM use) reports the statement's total: `Client::execute` returns 3 for a +/// 3-row insert. A per-row tag sequence makes the driver return the last +/// tag's `1`. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn multi_row_insert_extended_query_reports_row_count() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION tag_multi_ext (id INT PRIMARY KEY, v TEXT)") + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + + let affected = server + .client + .execute( + "INSERT INTO tag_multi_ext (id, v) VALUES (10, 'x'), (11, 'y'), (12, 'z')", + &[], + ) + .await + .unwrap_or_else(|e| panic!("extended-query insert: {e:?}")); + assert_eq!( + affected, 3, + "extended-query rows_affected must be the statement total, not the last row's 1" + ); +} + +/// The strict document engine takes the same per-row lowering and owes the +/// same single `INSERT 0 3`. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn strict_multi_row_insert_reports_one_tag_with_row_count() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION tag_multi_strict (id INT PRIMARY KEY, v TEXT) \ + WITH (engine='document_strict')", + ) + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + + assert_single_tag( + &server, + "INSERT INTO tag_multi_strict (id, v) VALUES (1, 'a'), (2, 'b'), (3, 'c')", + 3, + ) + .await; +} + +/// `INSERT ... ON CONFLICT DO NOTHING` over several rows reports how many +/// rows were actually written: with one key already present, `INSERT 0 2`. +/// The count is the sum of per-row outcomes, so a fold that assumes one row +/// per task would report 3. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn multi_row_insert_on_conflict_do_nothing_reports_rows_written() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION tag_multi_conflict (id INT PRIMARY KEY, v TEXT)") + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + server + .exec("INSERT INTO tag_multi_conflict (id, v) VALUES (2, 'existing')") + .await + .unwrap_or_else(|e| panic!("seed: {e}")); + + assert_single_tag( + &server, + "INSERT INTO tag_multi_conflict (id, v) VALUES (1, 'a'), (2, 'b'), (3, 'c') \ + ON CONFLICT DO NOTHING", + 2, + ) + .await; + assert_eq!( + live_rows(&server, "SELECT count(*) FROM tag_multi_conflict").await, + 3, + "the two new rows must have landed beside the existing one" + ); +} + +/// Inside an explicit transaction a multi-row insert is still one statement: +/// `INSERT 0 3` at statement time, exactly as Postgres reports it. The staged +/// (statement-time) write path must fold its per-row tags the same way the +/// autocommit path does. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn in_transaction_multi_row_insert_reports_one_tag_with_row_count() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION tag_multi_txn (id INT PRIMARY KEY, v TEXT)") + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + server + .exec("BEGIN") + .await + .unwrap_or_else(|e| panic!("begin: {e}")); + + assert_single_tag( + &server, + "INSERT INTO tag_multi_txn (id, v) VALUES (1, 'a'), (2, 'b'), (3, 'c')", + 3, + ) + .await; + + server + .exec("COMMIT") + .await + .unwrap_or_else(|e| panic!("commit: {e}")); + assert_eq!( + live_rows(&server, "SELECT count(*) FROM tag_multi_txn").await, + 3, + "all three rows must be visible after COMMIT" + ); +} + +/// Two statements in one simple-query buffer answer with two tags, one per +/// statement, in order. Folding must stop at the statement boundary — a fold +/// over the whole query buffer would collapse `1` and `2` into one `3`. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn multi_statement_query_reports_one_tag_per_statement() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION tag_multi_stmt (id INT PRIMARY KEY, v TEXT)") + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + + let (tags, _) = command_tags( + &server, + "INSERT INTO tag_multi_stmt (id, v) VALUES (1, 'a'); \ + INSERT INTO tag_multi_stmt (id, v) VALUES (2, 'b'), (3, 'c')", + ) + .await; + assert_eq!( + tags, + vec![1, 2], + "each statement in the buffer answers with its own tag and its own count" + ); +} + +/// A key-value `INSERT` answers `INSERT 0 1`, a Postgres command tag a +/// driver can read a count from — not a bare `OK`, which parses to `0` and +/// tells an ORM the write did not land. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kv_insert_reports_insert_tag_with_row_count() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION tag_kv (k TEXT PRIMARY KEY, v TEXT) WITH (engine='kv')") + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + + assert_single_tag(&server, "INSERT INTO tag_kv (k, v) VALUES ('a', '1')", 1).await; +} + +/// A multi-row key-value `INSERT` is one statement: one `INSERT 0 3`, not +/// three tags of any kind. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kv_multi_row_insert_reports_one_tag_with_row_count() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION tag_kv_multi (k TEXT PRIMARY KEY, v TEXT) WITH (engine='kv')") + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + + assert_single_tag( + &server, + "INSERT INTO tag_kv_multi (k, v) VALUES ('a', '1'), ('b', '2'), ('c', '3')", + 3, + ) + .await; + assert_eq!( + live_rows(&server, "SELECT count(*) FROM tag_kv_multi").await, + 3, + "all three keys must have landed" + ); +} + +/// Key-value `INSERT ... ON CONFLICT (k) DO UPDATE` reports a count whether +/// the row was inserted or updated: `INSERT 0 1` on first write, `UPDATE 1` +/// on overwrite. Both are countable; a bare `OK` is not. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kv_insert_on_conflict_do_update_reports_row_count() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION tag_kv_upsert (k TEXT PRIMARY KEY, n INT) WITH (engine='kv')") + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + + let sql = "INSERT INTO tag_kv_upsert (k, n) VALUES ('a', 1) \ + ON CONFLICT (k) DO UPDATE SET n = EXCLUDED.n"; + assert_single_tag(&server, sql, 1).await; + assert_single_tag(&server, sql, 1).await; +} + +/// A timeseries `INSERT` answers `INSERT 0 1`. The ingest handler already +/// reports how many rows it accepted; the count must reach the tag instead of +/// being replaced by a bare `OK`. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn timeseries_insert_reports_insert_tag_with_row_count() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION tag_ts \ + COLUMNS (id TEXT, ts BIGINT TIME_KEY, v INT) \ + WITH (engine='timeseries')", + ) + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + + assert_single_tag( + &server, + "INSERT INTO tag_ts (id, ts, v) VALUES ('a', 1000, 10)", + 1, + ) + .await; +} + +/// A multi-row timeseries `INSERT` lowers to one ingest task and answers +/// `INSERT 0 3` — the accepted-row count the ingest reports. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn timeseries_multi_row_insert_reports_one_tag_with_row_count() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION tag_ts_multi \ + COLUMNS (id TEXT, ts BIGINT TIME_KEY, v INT) \ + WITH (engine='timeseries')", + ) + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + + assert_single_tag( + &server, + "INSERT INTO tag_ts_multi (id, ts, v) \ + VALUES ('a', 1000, 10), ('b', 2000, 20), ('c', 3000, 30)", + 3, + ) + .await; +} + +/// A multi-row spatial `INSERT` (the shared columnar write path) answers one +/// `INSERT 0 3`. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn spatial_multi_row_insert_reports_one_tag_with_row_count() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION tag_spatial \ + COLUMNS (id TEXT, loc GEOMETRY) \ + WITH (engine='spatial')", + ) + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + + assert_single_tag( + &server, + "INSERT INTO tag_spatial (id, loc) VALUES \ + ('a', ST_MakePoint(1.0, 1.0)), \ + ('b', ST_MakePoint(2.0, 2.0)), \ + ('c', ST_MakePoint(3.0, 3.0))", + 3, + ) + .await; +} + +/// A vector-primary collection's `INSERT` answers `INSERT 0 1`, not `OK`. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn vector_primary_insert_reports_insert_tag_with_row_count() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION tag_vec (id STRING PRIMARY KEY, vec VECTOR(3), owner STRING) \ + WITH (engine='vector', primary='vector', vector_field='vec', dim=3, \ + payload_indexes=['owner'])", + ) + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + + assert_single_tag( + &server, + "INSERT INTO tag_vec (id, vec, owner) VALUES ('r1', ARRAY[1.0, 0.0, 0.0], 'alice')", + 1, + ) + .await; +} + +/// A CRDT collection's `INSERT` is a write statement: it answers `INSERT 0 1` +/// and no row data. Answering with the stored document (a `SELECT`-shaped +/// response) makes a driver's `execute` see rows where it expects a tag. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn crdt_insert_reports_insert_tag_with_row_count() { + let server = TestServer::start().await; + server + .exec("CREATE COLLECTION tag_crdt (id TEXT PRIMARY KEY, v INT) WITH (crdt='true')") + .await + .unwrap_or_else(|e| panic!("create collection: {e}")); + + assert_single_tag(&server, "INSERT INTO tag_crdt (id, v) VALUES ('a', 1)", 1).await; +} + +/// A multi-row `INSERT INTO ARRAY` answers one `INSERT 0 3`, not `OK`. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn array_multi_row_insert_reports_one_tag_with_row_count() { + let server = TestServer::start().await; + server + .exec( + "CREATE ARRAY tag_arr \ + DIMS (x INT64 [0..15]) \ + ATTRS (v INT64) \ + TILE_EXTENTS (16) \ + CELL_ORDER ROW_MAJOR", + ) + .await + .unwrap_or_else(|e| panic!("create array: {e}")); + + assert_single_tag( + &server, + "INSERT INTO ARRAY tag_arr \ + COORDS (0) VALUES (10), \ + COORDS (1) VALUES (11), \ + COORDS (2) VALUES (12)", + 3, + ) + .await; +} From 9e55c3950ef4195785e8fff7fce6bccf6415cc77 Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Sat, 19 Sep 2026 07:36:48 +0800 Subject: [PATCH 02/29] fix(resp): classify every client write as count-bearing on pgwire Writes on the KV, timeseries, vector-primary, CRDT, and cluster-array paths fell through describe_plan's engine wildcards to PlanKind::Execution, which pgwire renders as a bare OK tag: a driver parses that as zero rows affected. A CRDT INSERT/UPSERT/UPDATE was classified SingleDocument and answered with an empty row set instead of a command tag. Replace the wildcards with explicit per-variant arms so a new op forces a classification. KV Put answers UPSERT n (matching DocumentOp::Upsert on the autocommit, staged, and Calvin-folded paths); KV Insert/BatchPut, timeseries Ingest, vector-primary DirectUpsert, and array Put answer INSERT 0 n; array Delete answers DELETE n. Add PlanKind::DmlResultByOp for KV INSERT ... ON CONFLICT DO UPDATE, whose verb is decided per row: the handler reports {affected, op} and the response layer renders INSERT 0 n or UPDATE n from it. The RLS-gated resolve path for that op reports the same payload. Add CrdtWriteVerb to CrdtOp::DocUpsert, set by the planner from the statement (INSERT, UPSERT, UPDATE) and carried through WAL replication, so the tag follows the SQL verb. Emit the cluster-array coordinator's Put/Delete counts as a keyed map so the shared affected-count reader accepts them. Split response_shape/types/plan_kind.rs into one file per engine family plus a describe.rs dispatcher. --- .../src/physical_plan/crdt/collection.rs | 1 + nodedb-physical/src/physical_plan/crdt/mod.rs | 2 + nodedb-physical/src/physical_plan/crdt/op.rs | 4 + .../src/physical_plan/crdt/write_verb.rs | 40 ++ nodedb-physical/src/physical_plan/mod.rs | 2 +- .../cluster/array_cluster_exec/executor.rs | 12 +- nodedb/src/control/crdt_admission.rs | 1 + .../src/control/planner/rls_injection/crdt.rs | 1 + .../sql_plan_convert/dml/insert/convert.rs | 1 + .../dml/update_delete/update.rs | 1 + .../planner/sql_plan_convert/dml/upsert.rs | 1 + .../server/native/dispatch/conversion.rs | 9 +- .../server/native/dispatch/sql_loop.rs | 4 +- .../src/control/server/pgwire/handler/plan.rs | 81 ++- .../pgwire/handler/routing/calvin_response.rs | 19 +- .../pgwire/handler/routing/cluster_array.rs | 8 +- .../response_shape/compose/materialized.rs | 18 +- .../server/response_shape/types/plan_kind.rs | 478 ------------------ .../response_shape/types/plan_kind/array.rs | 33 ++ .../types/plan_kind/columnar_family.rs | 60 +++ .../response_shape/types/plan_kind/crdt.rs | 55 ++ .../types/plan_kind/describe.rs | 341 +++++++++++++ .../types/plan_kind/document.rs | 88 ++++ .../response_shape/types/plan_kind/graph.rs | 39 ++ .../response_shape/types/plan_kind/kind.rs | 28 + .../response_shape/types/plan_kind/kv.rs | 99 ++++ .../response_shape/types/plan_kind/mod.rs | 20 + .../response_shape/types/plan_kind/query.rs | 40 ++ .../response_shape/types/plan_kind/search.rs | 55 ++ .../server/shared/sql/staging_predicates.rs | 28 +- .../predicate/txn_buffering/classify.rs | 1 + .../src/control/server/wal_dispatch/crdt.rs | 2 + .../control/wal_replication/decode/crdt.rs | 36 +- .../wal_replication/decode/entry_crdt.rs | 26 +- .../control/wal_replication/encode/crdt.rs | 40 +- .../wal_replication/types/replicated_write.rs | 4 +- .../src/data/executor/core_loop/response.rs | 5 +- nodedb/src/data/executor/dispatch/crdt.rs | 2 + .../executor/handlers/kv/resolve/write_ops.rs | 11 +- .../data/executor/response_codec/encode.rs | 16 +- .../src/data/executor/response_codec/mod.rs | 4 +- .../src/data/executor/wal_replay/crdt_doc.rs | 17 +- 42 files changed, 1136 insertions(+), 597 deletions(-) create mode 100644 nodedb-physical/src/physical_plan/crdt/write_verb.rs delete mode 100644 nodedb/src/control/server/response_shape/types/plan_kind.rs create mode 100644 nodedb/src/control/server/response_shape/types/plan_kind/array.rs create mode 100644 nodedb/src/control/server/response_shape/types/plan_kind/columnar_family.rs create mode 100644 nodedb/src/control/server/response_shape/types/plan_kind/crdt.rs create mode 100644 nodedb/src/control/server/response_shape/types/plan_kind/describe.rs create mode 100644 nodedb/src/control/server/response_shape/types/plan_kind/document.rs create mode 100644 nodedb/src/control/server/response_shape/types/plan_kind/graph.rs create mode 100644 nodedb/src/control/server/response_shape/types/plan_kind/kind.rs create mode 100644 nodedb/src/control/server/response_shape/types/plan_kind/kv.rs create mode 100644 nodedb/src/control/server/response_shape/types/plan_kind/mod.rs create mode 100644 nodedb/src/control/server/response_shape/types/plan_kind/query.rs create mode 100644 nodedb/src/control/server/response_shape/types/plan_kind/search.rs diff --git a/nodedb-physical/src/physical_plan/crdt/collection.rs b/nodedb-physical/src/physical_plan/crdt/collection.rs index 1230620cf..9004ebef5 100644 --- a/nodedb-physical/src/physical_plan/crdt/collection.rs +++ b/nodedb-physical/src/physical_plan/crdt/collection.rs @@ -179,6 +179,7 @@ mod tests { fields_json: "{}".to_string(), surrogate: Surrogate::ZERO, partial: false, + verb: super::write_verb::CrdtWriteVerb::Insert, returning: None, rls_filters: Vec::new(), }, diff --git a/nodedb-physical/src/physical_plan/crdt/mod.rs b/nodedb-physical/src/physical_plan/crdt/mod.rs index 4c61a12e6..770af2fd1 100644 --- a/nodedb-physical/src/physical_plan/crdt/mod.rs +++ b/nodedb-physical/src/physical_plan/crdt/mod.rs @@ -4,5 +4,7 @@ pub mod collection; pub mod op; +pub mod write_verb; pub use op::CrdtOp; +pub use write_verb::CrdtWriteVerb; diff --git a/nodedb-physical/src/physical_plan/crdt/op.rs b/nodedb-physical/src/physical_plan/crdt/op.rs index 80c5a597f..202c287d9 100644 --- a/nodedb-physical/src/physical_plan/crdt/op.rs +++ b/nodedb-physical/src/physical_plan/crdt/op.rs @@ -6,6 +6,8 @@ use nodedb_types::{QualifiedCollection, Surrogate}; use crate::physical_plan::document::ReturningSpec; +use super::write_verb::CrdtWriteVerb; + /// CRDT engine physical operations. #[derive( Debug, @@ -224,6 +226,8 @@ pub enum CrdtOp { fields_json: String, surrogate: Surrogate, partial: bool, + /// The SQL statement that produced this write; decides the command tag. + verb: CrdtWriteVerb, /// When `Some`, return the STORED post-image of the upserted row — /// projected per spec. Carried across replication so a replay /// re-executes this write for the originating request, not just for diff --git a/nodedb-physical/src/physical_plan/crdt/write_verb.rs b/nodedb-physical/src/physical_plan/crdt/write_verb.rs new file mode 100644 index 000000000..da885c067 --- /dev/null +++ b/nodedb-physical/src/physical_plan/crdt/write_verb.rs @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: Apache-2.0 + +//! The SQL verb behind a `CrdtOp::DocUpsert`. +//! +//! `INSERT`, `UPSERT` and `UPDATE` against a CRDT collection all lower to the +//! same Loro map write. The verb rides on the op so the response layer can +//! render the command tag the client's statement expects. + +/// The SQL statement that produced a `CrdtOp::DocUpsert`. +#[derive( + Clone, + Copy, + Debug, + PartialEq, + Eq, + serde::Serialize, + serde::Deserialize, + zerompk::ToMessagePack, + zerompk::FromMessagePack, +)] +pub enum CrdtWriteVerb { + /// Plain `INSERT`: full-row replace, tagged `INSERT 0 n`. + Insert, + /// `UPSERT` / `INSERT ... ON CONFLICT DO UPDATE`: full-row replace, + /// tagged `UPSERT n`. + Upsert, + /// `UPDATE ... SET`: partial field write, tagged `UPDATE n`. + Update, +} + +impl CrdtWriteVerb { + /// The pgwire command-tag word for this verb. + pub fn command_tag(self) -> &'static str { + match self { + Self::Insert => "INSERT", + Self::Upsert => "UPSERT", + Self::Update => "UPDATE", + } + } +} diff --git a/nodedb-physical/src/physical_plan/mod.rs b/nodedb-physical/src/physical_plan/mod.rs index 0df5f1b35..fae149271 100644 --- a/nodedb-physical/src/physical_plan/mod.rs +++ b/nodedb-physical/src/physical_plan/mod.rs @@ -35,7 +35,7 @@ pub use array::{ArrayBinaryOp, ArrayOp, ArrayReducer}; pub use cluster_array::ClusterArrayOp; pub use cluster_event::{ClusterEventOp, MAX_REMOTE_CDC_COMMITTED_OFFSETS}; pub use columnar::{ColumnarInsertIntent, ColumnarOp}; -pub use crdt::CrdtOp; +pub use crdt::{CrdtOp, CrdtWriteVerb}; pub use document::{ BalancedDef, DocumentOp, DocumentResolveOutcome, DocumentResolvedMutation, EnforcementOptions, GeneratedColumnSpec, MaterializedSumBinding, OllpPredictedEdge, PeriodLockConfig, diff --git a/nodedb/src/control/cluster/array_cluster_exec/executor.rs b/nodedb/src/control/cluster/array_cluster_exec/executor.rs index 9308de4d1..b7ce0feb6 100644 --- a/nodedb/src/control/cluster/array_cluster_exec/executor.rs +++ b/nodedb/src/control/cluster/array_cluster_exec/executor.rs @@ -20,6 +20,7 @@ use crate::control::cluster::array_cluster_helpers::{ cluster_err, encode_err, finalize_agg_partials, }; use crate::control::state::SharedState; +use crate::data::executor::response_codec; use nodedb_physical::physical_plan::ClusterArrayOp; use zerompk; @@ -320,10 +321,9 @@ impl ClusterArrayExecutor { .await .map_err(cluster_err)?; - // Return a simple `{"affected": N}` JSON payload — same shape as the - // local ArrayOp::Put response so downstream decode is unchanged. - let affected = cells.len() as u64; - zerompk::to_msgpack_vec(&affected).map_err(encode_err) + // `{"inserted": n}`, the same count map the local `ArrayOp::Put` + // handler emits, so `extract_affected_count` reads both. + response_codec::encode_count("inserted", cells.len()) } async fn execute_delete( @@ -349,7 +349,7 @@ impl ClusterArrayExecutor { .await .map_err(cluster_err)?; - let deleted = coords.len() as u64; - zerompk::to_msgpack_vec(&deleted).map_err(encode_err) + // `{"deleted": n}`, matching the local `ArrayOp::Delete` handler. + response_codec::encode_count("deleted", coords.len()) } } diff --git a/nodedb/src/control/crdt_admission.rs b/nodedb/src/control/crdt_admission.rs index 2f881405b..726333486 100644 --- a/nodedb/src/control/crdt_admission.rs +++ b/nodedb/src/control/crdt_admission.rs @@ -1220,6 +1220,7 @@ mod tests { fields_json: "{}".into(), surrogate, partial: false, + verb: nodedb_physical::physical_plan::CrdtWriteVerb::Insert, returning: None, rls_filters: Vec::new(), }); diff --git a/nodedb/src/control/planner/rls_injection/crdt.rs b/nodedb/src/control/planner/rls_injection/crdt.rs index 8b918a237..7f80490f5 100644 --- a/nodedb/src/control/planner/rls_injection/crdt.rs +++ b/nodedb/src/control/planner/rls_injection/crdt.rs @@ -174,6 +174,7 @@ mod tests { fields_json: "{}".into(), surrogate: nodedb_types::Surrogate::ZERO, partial: false, + verb: nodedb_physical::physical_plan::CrdtWriteVerb::Insert, returning: None, rls_filters: Vec::new(), }); diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/insert/convert.rs b/nodedb/src/control/planner/sql_plan_convert/dml/insert/convert.rs index 970986c4e..332605e35 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/insert/convert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/insert/convert.rs @@ -126,6 +126,7 @@ pub(in super::super::super) fn convert_insert( fields_json: super::super::crdt_gate::row_to_fields_json(row)?, surrogate, partial: false, + verb: CrdtWriteVerb::Insert, returning: None, rls_filters: Vec::new(), }) diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs b/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs index 3f31e4027..9f032058f 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs @@ -272,6 +272,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_update( fields_json: fields_json.clone(), surrogate, partial: true, + verb: CrdtWriteVerb::Update, returning: None, rls_filters: Vec::new(), }) diff --git a/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs b/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs index b93b0fed7..f37ecf15a 100644 --- a/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/dml/upsert.rs @@ -103,6 +103,7 @@ pub(in super::super) fn convert_upsert( fields_json: super::crdt_gate::row_to_fields_json(row)?, surrogate, partial: false, + verb: CrdtWriteVerb::Upsert, returning: None, rls_filters: Vec::new(), }) diff --git a/nodedb/src/control/server/native/dispatch/conversion.rs b/nodedb/src/control/server/native/dispatch/conversion.rs index 3df2f2c86..8dc8d0ded 100644 --- a/nodedb/src/control/server/native/dispatch/conversion.rs +++ b/nodedb/src/control/server/native/dispatch/conversion.rs @@ -219,9 +219,12 @@ pub(crate) fn calvin_native_response( let returning_plan = plans .iter() .find(|p| matches!(describe_plan(p), PlanKind::ReturningRows)); - let dml_plan = plans - .iter() - .find(|p| matches!(describe_plan(p), PlanKind::DmlResult(_))); + let dml_plan = plans.iter().find(|p| { + matches!( + describe_plan(p), + PlanKind::DmlResult(_) | PlanKind::DmlResultByOp + ) + }); let redaction = returning_plan.map(|plan| QueryRedaction::for_plan(tenant_id, auth, plan)); if let (Some(resp), Some(plan)) = (apply_result.as_ref(), returning_plan) diff --git a/nodedb/src/control/server/native/dispatch/sql_loop.rs b/nodedb/src/control/server/native/dispatch/sql_loop.rs index 9905d199c..7a7da78e4 100644 --- a/nodedb/src/control/server/native/dispatch/sql_loop.rs +++ b/nodedb/src/control/server/native/dispatch/sql_loop.rs @@ -296,7 +296,9 @@ pub(super) async fn run_dispatch_loop( // the loop. let mut task_rows: Option = None; let plan_kind = describe_plan(&plan_for_response); - if let crate::control::server::response_shape::types::PlanKind::DmlResult(_) = plan_kind { + if let crate::control::server::response_shape::types::PlanKind::DmlResult(_) + | crate::control::server::response_shape::types::PlanKind::DmlResultByOp = plan_kind + { // A count-bearing write must report the rows it actually touched. // Adding 1 per dispatched task instead, as the empty-payload // branch below does, would report a row for a delete that removed diff --git a/nodedb/src/control/server/pgwire/handler/plan.rs b/nodedb/src/control/server/pgwire/handler/plan.rs index 84a8f34f4..d03ade792 100644 --- a/nodedb/src/control/server/pgwire/handler/plan.rs +++ b/nodedb/src/control/server/pgwire/handler/plan.rs @@ -14,7 +14,7 @@ use crate::data::executor::response_codec::decode_payload_to_json; use nodedb_physical::physical_plan::DocumentOp; use crate::control::server::shared::sql::staging_predicates::{ - StagedTagKind, require_affected_count, + StagedTagKind, extract_kv_conflict_op, require_affected_count, }; use super::super::command_tag::dml_tag; @@ -34,7 +34,8 @@ pub(super) use crate::control::server::response_shape::types::{PlanKind, describ /// /// **Foldable** — writes that unconditionally apply one row: /// - `PointPut` (Document) → INSERT 0 1 (upsert: always writes) -/// - `KvOp::Put` → INSERT 0 1 (upsert: always writes) +/// - `KvOp::Put` → UPSERT 1 (upsert: always writes; tagged like +/// `DocumentOp::Upsert`, the SQL `UPSERT` statement both lower from) /// /// **Not foldable**: /// - `PointDelete`, `PointUpdate`, `KvOp::Delete` — no-op when the target row @@ -97,11 +98,12 @@ pub(super) fn tag_from_staged(kind: StagedTagKind, affected: usize) -> Tag { StagedTagKind::Delete => dml_tag("DELETE", affected), StagedTagKind::KvUpsert { updated: true } => dml_tag("UPDATE", affected), StagedTagKind::KvUpsert { updated: false } => dml_tag("INSERT", affected), - // Matches the autocommit `DocumentOp::Upsert` tag exactly: always the - // literal `UPSERT` command, regardless of insert-vs-update outcome - // (see `response_shape::types::describe_plan`'s `DmlResult("UPSERT")` - // arm and `payload_to_response`'s `PlanKind::DmlResult` rendering). - StagedTagKind::DocUpsert => dml_tag("UPSERT", affected), + // Matches the autocommit `DocumentOp::Upsert` / `KvOp::Put` tag + // exactly: always the literal `UPSERT` command, regardless of + // insert-vs-update outcome (see `response_shape::types::describe_plan`'s + // `DmlResult("UPSERT")` arms and `payload_to_response`'s + // `PlanKind::DmlResult` rendering). + StagedTagKind::Upsert => dml_tag("UPSERT", affected), // Statement-time in-transaction MERGE: the Postgres command tag for a // MERGE is `MERGE ` across all arms. StagedTagKind::Merge => dml_tag("MERGE", affected), @@ -128,8 +130,10 @@ pub(super) fn calvin_tag_for_plan(plan: &PhysicalPlan) -> PgWireResult { use nodedb_physical::physical_plan::KvOp; match plan { - PhysicalPlan::Document(DocumentOp::PointPut { .. }) - | PhysicalPlan::Kv(KvOp::Put { .. }) => Ok(dml_tag("INSERT", 1)), + PhysicalPlan::Document(DocumentOp::PointPut { .. }) => Ok(dml_tag("INSERT", 1)), + // The SQL `UPSERT` statement: same tag as its `DocumentOp::Upsert` + // sibling and as `describe_plan`'s `DmlResult("UPSERT")` arm. + PhysicalPlan::Kv(KvOp::Put { .. }) => Ok(dml_tag("UPSERT", 1)), other => Err(invalid_plan_shape(format!( "calvin_tag_for_plan called on non-foldable plan: {other:?}" @@ -170,6 +174,27 @@ pub(super) fn payload_to_response(payload: &[u8], kind: PlanKind) -> PgWireResul })? as usize; Ok(Response::Execution(dml_tag(tag, count)).into()) } + PlanKind::DmlResultByOp => { + let count = require_affected_count(payload).map_err(|e| { + invalid_plan_shape(format!( + "DmlResultByOp response is missing its affected count: {e}" + )) + })? as usize; + // The handler decides insert-vs-update at apply time and reports + // it as `op`. A missing or unknown verb is a handler bug, never a + // default tag. + let tag = match extract_kv_conflict_op(payload).as_deref() { + Some("insert") => "INSERT", + Some("update") => "UPDATE", + other => { + return Err(invalid_plan_shape(format!( + "DmlResultByOp response carries no usable `op` verb \ + (got {other:?}); the handler must report `insert` or `update`" + ))); + } + }; + Ok(Response::Execution(dml_tag(tag, count)).into()) + } PlanKind::ArraySlice | PlanKind::ReturningRows | PlanKind::SingleDocument => { Err(invalid_plan_shape(format!( "payload_to_response cannot handle plan kind {kind:?}" @@ -264,7 +289,43 @@ mod tests { rls_filters: Vec::new(), }); assert!(is_calvin_foldable(&plan)); - assert!(calvin_tag_for_plan(&plan).is_ok()); + let tag: pgwire::messages::response::CommandComplete = calvin_tag_for_plan(&plan) + .expect("foldable plan renders a tag") + .into(); + assert_eq!(tag.tag, "UPSERT 1"); + } + + /// `KvOp::InsertOnConflictUpdate` reports the verb it resolved to; the tag + /// follows it, and a payload with no verb is refused rather than defaulted. + #[test] + fn dml_result_by_op_follows_the_reported_verb() { + let update = nodedb_types::json_to_msgpack(&serde_json::json!({ + "affected": 1, + "op": "update" + })) + .expect("encode payload"); + let shaped = payload_to_response(&update, PlanKind::DmlResultByOp).expect("update tag"); + let Response::Execution(tag) = shaped.response else { + panic!("expected an execution tag"); + }; + let tag: pgwire::messages::response::CommandComplete = tag.into(); + assert_eq!(tag.tag, "UPDATE 1"); + + let insert = nodedb_types::json_to_msgpack(&serde_json::json!({ + "affected": 1, + "op": "insert" + })) + .expect("encode payload"); + let shaped = payload_to_response(&insert, PlanKind::DmlResultByOp).expect("insert tag"); + let Response::Execution(tag) = shaped.response else { + panic!("expected an execution tag"); + }; + let tag: pgwire::messages::response::CommandComplete = tag.into(); + assert_eq!(tag.tag, "INSERT 0 1"); + + let no_verb = nodedb_types::json_to_msgpack(&serde_json::json!({ "affected": 1 })) + .expect("encode payload"); + assert!(payload_to_response(&no_verb, PlanKind::DmlResultByOp).is_err()); } /// A write that can legitimately touch nothing must NOT be folded: its count diff --git a/nodedb/src/control/server/pgwire/handler/routing/calvin_response.rs b/nodedb/src/control/server/pgwire/handler/routing/calvin_response.rs index bbb3fb614..d4ffa8120 100644 --- a/nodedb/src/control/server/pgwire/handler/routing/calvin_response.rs +++ b/nodedb/src/control/server/pgwire/handler/routing/calvin_response.rs @@ -97,7 +97,18 @@ pub(super) fn calvin_execution_response( // submit's RPC reply), so a count-bearing plan ALWAYS has one here. If it // does not, the deposit path regressed: fail loudly rather than synthesise a // count, which is what made a delete of an absent row report a removed row. - if let PlanKind::DmlResult(tag) = describe_plan(&task.plan) { + let plan_kind = describe_plan(&task.plan); + let count_bearing_tag = match plan_kind { + PlanKind::DmlResult(tag) => Some(tag), + // The verb is in the payload; the error text below only names the kind. + PlanKind::DmlResultByOp => Some("insert-or-update"), + PlanKind::Execution + | PlanKind::ArraySlice + | PlanKind::ReturningRows + | PlanKind::SingleDocument + | PlanKind::MultiRow => None, + }; + if let Some(tag) = count_bearing_tag { let resp = apply_resp.ok_or_else(|| { PgWireError::UserError(Box::new(ErrorInfo::new( "ERROR".to_owned(), @@ -109,11 +120,7 @@ pub(super) fn calvin_execution_response( ))) })?; return Ok(CalvinTaskOutcome::Tag( - super::super::plan::payload_to_response( - resp.payload.as_bytes(), - describe_plan(&task.plan), - )? - .response, + super::super::plan::payload_to_response(resp.payload.as_bytes(), plan_kind)?.response, )); } diff --git a/nodedb/src/control/server/pgwire/handler/routing/cluster_array.rs b/nodedb/src/control/server/pgwire/handler/routing/cluster_array.rs index b77b9a375..7d754f84f 100644 --- a/nodedb/src/control/server/pgwire/handler/routing/cluster_array.rs +++ b/nodedb/src/control/server/pgwire/handler/routing/cluster_array.rs @@ -107,9 +107,11 @@ impl NodeDbPgHandler { let cluster_plan_kind = match &cluster_op { ClusterArrayOp::Slice { .. } => PlanKind::ArraySlice, - ClusterArrayOp::Agg { .. } - | ClusterArrayOp::Put { .. } - | ClusterArrayOp::Delete { .. } => PlanKind::MultiRow, + ClusterArrayOp::Agg { .. } => PlanKind::MultiRow, + // The coordinator reports `{"inserted": n}` / `{"deleted": n}`, + // the same count map the local array handlers emit. + ClusterArrayOp::Put { .. } => PlanKind::DmlResult("INSERT"), + ClusterArrayOp::Delete { .. } => PlanKind::DmlResult("DELETE"), }; // This coordinator path never builds a `PhysicalPlan`, so the source // collection comes straight off the op's array name. A single source diff --git a/nodedb/src/control/server/response_shape/compose/materialized.rs b/nodedb/src/control/server/response_shape/compose/materialized.rs index b454ed801..ce00ae8fd 100644 --- a/nodedb/src/control/server/response_shape/compose/materialized.rs +++ b/nodedb/src/control/server/response_shape/compose/materialized.rs @@ -37,9 +37,9 @@ use super::kernel::{empty_shaped, shape_decoded_rows, single_result_row}; /// /// Row-producing plan kinds (`SingleDocument`, `MultiRow`, `ReturningRows`, /// `ArraySlice`) yield `Rows`. Tag/execution kinds (`Execution`, -/// `DmlResult`) yield `Passthrough` — a `ShapedRows` cannot represent a bare -/// `CommandComplete` tag or affected-row count, so callers keep their -/// existing tag / `rows_affected` handling for those. +/// `DmlResult`, `DmlResultByOp`) yield `Passthrough` — a `ShapedRows` cannot +/// represent a bare `CommandComplete` tag or affected-row count, so callers +/// keep their existing tag / `rows_affected` handling for those. pub enum ShapeOutcome { Rows(ShapedRows), Passthrough, @@ -65,7 +65,9 @@ pub fn shape_response_materialized( } = request; match plan_kind { - PlanKind::Execution | PlanKind::DmlResult(_) => return Ok(ShapeOutcome::Passthrough), + PlanKind::Execution | PlanKind::DmlResult(_) | PlanKind::DmlResultByOp => { + return Ok(ShapeOutcome::Passthrough); + } PlanKind::ArraySlice | PlanKind::ReturningRows | PlanKind::SingleDocument @@ -90,7 +92,9 @@ pub fn shape_response_materialized( // Handled by the early return above; kept exhaustive (no catch-all, // no panic) so a future PlanKind desync degrades to passthrough // rather than crashing the connection. - PlanKind::Execution | PlanKind::DmlResult(_) => return Ok(ShapeOutcome::Passthrough), + PlanKind::Execution | PlanKind::DmlResult(_) | PlanKind::DmlResultByOp => { + return Ok(ShapeOutcome::Passthrough); + } }; Ok(ShapeOutcome::Rows(shaped)) } @@ -115,7 +119,9 @@ pub fn shape_payload_no_plan( sequences: Option<&dyn SequenceAccess>, ) -> Result { Ok(match plan_kind { - PlanKind::Execution | PlanKind::DmlResult(_) => ShapeOutcome::Passthrough, + PlanKind::Execution | PlanKind::DmlResult(_) | PlanKind::DmlResultByOp => { + ShapeOutcome::Passthrough + } PlanKind::ArraySlice => ShapeOutcome::Rows(shape_array_slice(payload, redaction)?), PlanKind::ReturningRows => ShapeOutcome::Rows(shape_returning_rows( payload, projection, redaction, sequences, diff --git a/nodedb/src/control/server/response_shape/types/plan_kind.rs b/nodedb/src/control/server/response_shape/types/plan_kind.rs deleted file mode 100644 index 93476319a..000000000 --- a/nodedb/src/control/server/response_shape/types/plan_kind.rs +++ /dev/null @@ -1,478 +0,0 @@ -// SPDX-License-Identifier: BUSL-1.1 - -//! Protocol-neutral plan classification types. -//! -//! These operate purely on `PhysicalPlan` and carry no pgwire wire types, -//! so they are shared across any protocol-specific response shaper. - -use crate::bridge::envelope::PhysicalPlan; -use nodedb_physical::physical_plan::{ - ColumnarOp, CrdtOp, DocumentOp, GraphOp, KvOp, QueryOp, SpatialOp, TextOp, TimeseriesOp, - VectorOp, -}; - -#[derive(Debug, Clone, Copy)] -pub enum PlanKind { - SingleDocument, - MultiRow, - /// Array slice result — decoded via `ArraySliceResponse` to surface the - /// `truncated_before_horizon` flag as a pgwire NOTICE when set. - ArraySlice, - Execution, - /// DML operation that returns affected row count. - /// The tag name is used in the pgwire `CommandComplete` message (e.g., "UPDATE", "DELETE"). - DmlResult(&'static str), - /// DML with RETURNING clause — payload is a `RowsPayload` (msgpack). - /// Decoded into one pgwire field per column. - ReturningRows, -} - -pub fn describe_plan(plan: &PhysicalPlan) -> PlanKind { - match plan { - PhysicalPlan::Crdt(CrdtOp::DocUpsert { - returning: Some(_), .. - }) - | PhysicalPlan::Crdt(CrdtOp::DocDelete { - returning: Some(_), .. - }) => PlanKind::ReturningRows, - - // A CRDT delete can legitimately remove nothing, so its count must render - // as a DML count from the write's own response, not a document-shaped read. - PhysicalPlan::Crdt(CrdtOp::DocDelete { .. }) => DmlResult("DELETE"), - - PhysicalPlan::Document(DocumentOp::PointGet { .. }) - | PhysicalPlan::Crdt(CrdtOp::Read { .. }) - | PhysicalPlan::Crdt(CrdtOp::GetPolicy { .. }) - | PhysicalPlan::Crdt(CrdtOp::DocUpsert { .. }) => PlanKind::SingleDocument, - - PhysicalPlan::Vector(VectorOp::Search { .. }) - | PhysicalPlan::Vector(VectorOp::MultiSearch { .. }) - | PhysicalPlan::Vector(VectorOp::MultiVectorScoreSearch { .. }) - | PhysicalPlan::Vector(VectorOp::SparseSearch { .. }) - | PhysicalPlan::Document(DocumentOp::RangeScan { .. }) - | PhysicalPlan::Graph(GraphOp::Hop { .. }) - | PhysicalPlan::Graph(GraphOp::Neighbors { .. }) - | PhysicalPlan::Graph(GraphOp::Path { .. }) - | PhysicalPlan::Graph(GraphOp::Subgraph { .. }) - | PhysicalPlan::Graph(GraphOp::RagFusion { .. }) - | PhysicalPlan::Document(DocumentOp::Scan { .. }) - | PhysicalPlan::Document(DocumentOp::IndexedFetch { .. }) - | PhysicalPlan::Columnar(ColumnarOp::Scan { .. }) - | PhysicalPlan::Timeseries(TimeseriesOp::Scan { .. }) - | PhysicalPlan::Spatial(SpatialOp::Scan { .. }) - | PhysicalPlan::Kv(KvOp::Scan { .. }) - | PhysicalPlan::Kv(KvOp::BatchGet { .. }) - | PhysicalPlan::Query(QueryOp::Aggregate { .. }) - | PhysicalPlan::Query(QueryOp::FacetCounts { .. }) - | PhysicalPlan::Query(QueryOp::HashJoin { .. }) - | PhysicalPlan::Query(QueryOp::RecursiveScan { .. }) - | PhysicalPlan::Query(QueryOp::RecursiveValue { .. }) - | PhysicalPlan::Query(QueryOp::LateralTopK { .. }) - | PhysicalPlan::Query(QueryOp::LateralLoop { .. }) - | PhysicalPlan::Graph(GraphOp::Algo { .. }) - | PhysicalPlan::Graph(GraphOp::Match { .. }) - | PhysicalPlan::Graph(GraphOp::MatchContinuation { .. }) - | PhysicalPlan::Graph(GraphOp::MatchVarLenResume { .. }) - | PhysicalPlan::Graph(GraphOp::BspSuperstep(_)) - | PhysicalPlan::Graph(GraphOp::WccSuperstep(_)) - | PhysicalPlan::Text(TextOp::Search { .. }) - | PhysicalPlan::Text(TextOp::PhraseSearch { .. }) - | PhysicalPlan::Text(TextOp::HybridSearch { .. }) - | PhysicalPlan::Text(TextOp::HybridSearchTriple { .. }) - | PhysicalPlan::Text(TextOp::BM25ScoreScan { .. }) - | PhysicalPlan::Text(TextOp::FtsIndexDoc { .. }) - | PhysicalPlan::Text(TextOp::FtsDeleteDoc { .. }) => PlanKind::MultiRow, - - // Opaque execution results: config write, index teardown status. - PhysicalPlan::Text(TextOp::SetTextConfig { .. }) - | PhysicalPlan::Vector(VectorOp::DropIndex { .. }) - // Internal typed zerompk value, never a client row — decoded by the - // admission caller as `CrdtPreviewResult`. - | PhysicalPlan::Crdt(CrdtOp::PreviewApply { .. }) => PlanKind::Execution, - - PhysicalPlan::Kv(KvOp::Get { .. }) | PhysicalPlan::Kv(KvOp::FieldGet { .. }) => { - PlanKind::SingleDocument - } - - // Constant/catalog-scan expressions compile to ProviderScan; route MultiRow - // so each array element streams as its own pgwire row. - PhysicalPlan::Query(QueryOp::ProviderScan { .. }) => PlanKind::MultiRow, - - // Exchange means the plan wasn't yet resolved — recurse into the child. - PhysicalPlan::Query(QueryOp::Exchange(op)) => describe_plan(&op.child), - - // PostProcess reshapes a multi-row subquery; its kind is the child's. - PhysicalPlan::Query(QueryOp::PostProcess { input, .. }) => describe_plan(input), - - // SetOp resolves to a ProviderScan of merged rows; route MultiRow so - // each row streams as its own pgwire row. - PhysicalPlan::Query(QueryOp::SetOp { .. }) => PlanKind::MultiRow, - - // An insert with a projection returns real stored rows and must be decoded - // and redacted, else it silently leaks unredacted rows like `Merge` did. - PhysicalPlan::Kv( - KvOp::Insert { - returning: Some(_), .. - } - | KvOp::InsertIfAbsent { - returning: Some(_), .. - } - | KvOp::InsertOnConflictUpdate { - returning: Some(_), .. - } - | KvOp::Put { - returning: Some(_), .. - } - | KvOp::BatchPut { - returning: Some(_), .. - }, - ) - | PhysicalPlan::Document(DocumentOp::PointPut { - returning: Some(_), .. - }) - | PhysicalPlan::Document(DocumentOp::PointInsert { - returning: Some(_), .. - }) - | PhysicalPlan::Document(DocumentOp::BatchInsert { - returning: Some(_), .. - }) - | PhysicalPlan::Columnar(ColumnarOp::Insert { - returning: Some(_), .. - }) - | PhysicalPlan::Timeseries(TimeseriesOp::Ingest { - returning: Some(_), .. - }) - | PhysicalPlan::Vector(VectorOp::DirectUpsert { - returning: Some(_), .. - }) => PlanKind::ReturningRows, - - // `PointInsert`/`InsertIfAbsent`: `ON CONFLICT DO NOTHING` makes them - // no-op-capable, so the count must come from the write's response. - PhysicalPlan::Document(DocumentOp::PointPut { .. }) - | PhysicalPlan::Document(DocumentOp::PointInsert { .. }) - | PhysicalPlan::Document(DocumentOp::BatchInsert { .. }) - | PhysicalPlan::Kv(KvOp::InsertIfAbsent { .. }) - | PhysicalPlan::Columnar(ColumnarOp::Insert { .. }) => DmlResult("INSERT"), - - PhysicalPlan::Document(DocumentOp::PointUpdate { - returning: Some(_), .. - }) - | PhysicalPlan::Document(DocumentOp::BulkUpdate { - returning: Some(_), .. - }) => PlanKind::ReturningRows, - PhysicalPlan::Document(DocumentOp::PointUpdate { .. }) - | PhysicalPlan::Document(DocumentOp::BulkUpdate { .. }) => DmlResult("UPDATE"), - - PhysicalPlan::Document(DocumentOp::PointDelete { - returning: Some(_), .. - }) - | PhysicalPlan::Document(DocumentOp::BulkDelete { - returning: Some(_), .. - }) => PlanKind::ReturningRows, - PhysicalPlan::Document(DocumentOp::PointDelete { .. }) - | PhysicalPlan::Document(DocumentOp::BulkDelete { .. }) => DmlResult("DELETE"), - - PhysicalPlan::Document(DocumentOp::UpdateFromJoin { - returning: Some(_), .. - }) => PlanKind::ReturningRows, - PhysicalPlan::Document(DocumentOp::UpdateFromJoin { .. }) => DmlResult("UPDATE"), - - // A MERGE with a projection returns real target rows and must be decoded - // and redacted, else it falls through to unredacted `Execution` passthrough. - PhysicalPlan::Document(DocumentOp::Merge { - returning: Some(_), .. - }) => PlanKind::ReturningRows, - // Postgres tags a plain MERGE `MERGE `, matching the staged path. - PhysicalPlan::Document(DocumentOp::Merge { .. }) => DmlResult("MERGE"), - - PhysicalPlan::Document(DocumentOp::Truncate { .. }) => DmlResult("TRUNCATE"), - - // A KV update/delete with a projection returns real stored rows and - // must be decoded and redacted, exactly like the KV insert ops above. - PhysicalPlan::Kv( - KvOp::FieldSet { - returning: Some(_), .. - } - | KvOp::PredicateUpdate { - returning: Some(_), .. - } - | KvOp::Delete { - returning: Some(_), .. - } - | KvOp::PredicateDelete { - returning: Some(_), .. - }, - ) => PlanKind::ReturningRows, - // KV delete/truncate count the keys removed — `Execution` would discard that. - PhysicalPlan::Kv(KvOp::Delete { .. }) | PhysicalPlan::Kv(KvOp::PredicateDelete { .. }) => { - DmlResult("DELETE") - } - // Reports `{"affected": n}` — `Execution` would discard that count. - // `FieldSet` is the keyed UPDATE, so it tags the same way. - PhysicalPlan::Kv(KvOp::FieldSet { .. }) | PhysicalPlan::Kv(KvOp::PredicateUpdate { .. }) => { - DmlResult("UPDATE") - } - PhysicalPlan::Kv(KvOp::Truncate { .. }) => DmlResult("TRUNCATE"), - - PhysicalPlan::Document(DocumentOp::InsertSelect { .. }) => DmlResult("INSERT"), - - PhysicalPlan::Document(DocumentOp::Upsert { - returning: Some(_), .. - }) => PlanKind::ReturningRows, - PhysicalPlan::Document(DocumentOp::Upsert { .. }) => DmlResult("UPSERT"), - - // Array read/maintenance ops produce a JSON-array payload; route to the - // multi-row decoder so each row streams as its own pgwire field. - PhysicalPlan::Array(nodedb_physical::physical_plan::ArrayOp::Slice { .. }) => { - PlanKind::ArraySlice - } - PhysicalPlan::Array(nodedb_physical::physical_plan::ArrayOp::Project { .. }) - | PhysicalPlan::Array(nodedb_physical::physical_plan::ArrayOp::Aggregate { .. }) - | PhysicalPlan::Array(nodedb_physical::physical_plan::ArrayOp::Elementwise { .. }) => { - PlanKind::MultiRow - } - // Flush/Compact return status JSON — route SingleDocument. - PhysicalPlan::Array(nodedb_physical::physical_plan::ArrayOp::Flush { .. }) - | PhysicalPlan::Array(nodedb_physical::physical_plan::ArrayOp::Compact { .. }) => { - PlanKind::SingleDocument - } - - // Vector write/config ops carry no row payload. Enumerated explicitly (not a - // `Vector(_)` wildcard) so a future read op can't silently strand its hits. - PhysicalPlan::Vector(VectorOp::Insert { .. }) - | PhysicalPlan::Vector(VectorOp::BatchInsert { .. }) - | PhysicalPlan::Vector(VectorOp::Delete { .. }) - | PhysicalPlan::Vector(VectorOp::DeleteBySurrogate { .. }) - | PhysicalPlan::Vector(VectorOp::SetParams { .. }) - | PhysicalPlan::Vector(VectorOp::QueryStats { .. }) - | PhysicalPlan::Vector(VectorOp::Seal { .. }) - | PhysicalPlan::Vector(VectorOp::CompactIndex { .. }) - | PhysicalPlan::Vector(VectorOp::Rebuild { .. }) - | PhysicalPlan::Vector(VectorOp::SparseInsert { .. }) - | PhysicalPlan::Vector(VectorOp::SparseDelete { .. }) - | PhysicalPlan::Vector(VectorOp::MultiVectorInsert { .. }) - | PhysicalPlan::Vector(VectorOp::MultiVectorDelete { .. }) - | PhysicalPlan::Vector(VectorOp::DirectUpsert { .. }) => PlanKind::Execution, - - // Document ops with no row payload. Enumerated explicitly, not a `Document(_)` - // wildcard — that let `Merge` default to unredacted passthrough. - PhysicalPlan::Document(DocumentOp::Register { .. }) - | PhysicalPlan::Document(DocumentOp::IndexLookup { .. }) - | PhysicalPlan::Document(DocumentOp::DropIndex { .. }) - | PhysicalPlan::Document(DocumentOp::BackfillIndex { .. }) - | PhysicalPlan::Document(DocumentOp::EstimateCount { .. }) - | PhysicalPlan::Document(DocumentOp::MaterializeScan { .. }) - // Read-only resolve: payload is the internal classification tuple, never a client row. - | PhysicalPlan::Document(DocumentOp::ResolveWrite(_)) - // A derived balance write answers no client — reports an affected count only. - | PhysicalPlan::Document(DocumentOp::ApplyBalanceDelta { .. }) - // Never reaches this classifier: write-resolve returns the response itself, - // shaped from the intercepted plan whose `returning` slot decides. - | PhysicalPlan::Document(DocumentOp::ResolvedWrite { .. }) - - // Default: opaque execution result. Exhaustive so a new variant forces a decision. - | PhysicalPlan::Graph(_) - | PhysicalPlan::Kv(_) - | PhysicalPlan::Columnar(_) - | PhysicalPlan::Timeseries(_) - | PhysicalPlan::Spatial(_) - | PhysicalPlan::Crdt(_) - | PhysicalPlan::Query(_) - | PhysicalPlan::Meta(_) - | PhysicalPlan::Array(_) - | PhysicalPlan::ClusterArray(_) - | PhysicalPlan::ClusterEvent(_) => PlanKind::Execution, - } -} - -// Bring the variant into scope for brevity in match arms above. -use PlanKind::DmlResult; - -#[cfg(test)] -mod tests { - use super::*; - use nodedb_types::{DatabaseId, QualifiedCollection}; - - #[test] - fn crdt_preview_is_an_opaque_execution_plan() { - let plan = PhysicalPlan::Crdt(CrdtOp::PreviewApply { - collection: QualifiedCollection::new(DatabaseId::DEFAULT, "tasks"), - document_id: "task-1".to_string(), - delta: vec![0x92, 0x01], - }); - - assert!(matches!(describe_plan(&plan), PlanKind::Execution)); - } - - fn merge_plan( - returning: Option, - ) -> PhysicalPlan { - PhysicalPlan::Document(DocumentOp::Merge { - target_collection: QualifiedCollection::new(DatabaseId::DEFAULT, "target"), - source_collection: QualifiedCollection::new(DatabaseId::DEFAULT, "source"), - source_alias: "s".to_string(), - target_join_col: "id".to_string(), - source_join_col: "id".to_string(), - clauses: Vec::new(), - returning, - resolved_inserts: None, - source_rows: None, - rls_filters: Vec::new(), - rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), - resolved_sum_targets: Vec::new(), - declared_primary_key: None, - }) - } - - /// A `MERGE ... RETURNING` payload is real target rows — `Execution` would - /// pass them unredacted. - #[test] - fn merge_with_returning_is_returning_rows() { - use nodedb_physical::physical_plan::{ReturningColumns, ReturningSpec}; - - let plan = merge_plan(Some(ReturningSpec { - columns: ReturningColumns::Star, - })); - - assert!(matches!(describe_plan(&plan), PlanKind::ReturningRows)); - } - - /// Every insert-family op with a projection must classify row-returning, - /// else it leaks unredacted like the MERGE case above. - #[test] - fn inserts_with_returning_are_returning_rows() { - use nodedb_physical::physical_plan::{ReturningColumns, ReturningSpec}; - - let spec = || { - Some(ReturningSpec { - columns: ReturningColumns::Star, - }) - }; - let plans = [ - PhysicalPlan::Document(DocumentOp::PointInsert { - collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), - document_id: "d".into(), - value: Vec::new(), - if_absent: false, - surrogate: nodedb_types::Surrogate::ZERO, - returning: spec(), - rls_filters: Vec::new(), - resolved_sum_targets: Vec::new(), - deferred_sum_targets: Vec::new(), - }), - PhysicalPlan::Document(DocumentOp::PointPut { - collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), - document_id: "d".into(), - value: Vec::new(), - surrogate: nodedb_types::Surrogate::ZERO, - pk_bytes: Vec::new(), - returning: spec(), - rls_filters: Vec::new(), - resolved_sum_targets: Vec::new(), - }), - PhysicalPlan::Document(DocumentOp::BatchInsert { - collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), - documents: Vec::new(), - surrogates: Vec::new(), - returning: spec(), - rls_filters: Vec::new(), - resolved_sum_targets: Vec::new(), - deferred_sum_targets: Vec::new(), - }), - PhysicalPlan::Document(DocumentOp::Upsert { - collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), - document_id: "d".into(), - value: Vec::new(), - on_conflict_updates: Vec::new(), - surrogate: nodedb_types::Surrogate::ZERO, - rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), - returning: spec(), - rls_filters: Vec::new(), - resolved_sum_targets: Vec::new(), - }), - ]; - for plan in &plans { - assert!( - matches!(describe_plan(plan), PlanKind::ReturningRows), - "{plan:?} must shape as rows" - ); - } - } - - /// Every KV insert-family op that can carry a projection must classify as - /// row-returning too — the same passthrough leak, one engine over. - #[test] - fn kv_inserts_with_returning_are_returning_rows() { - use nodedb_physical::physical_plan::{KvOp, ReturningColumns, ReturningSpec}; - - let spec = || { - Some(ReturningSpec { - columns: ReturningColumns::Star, - }) - }; - let plans = [ - PhysicalPlan::Kv(KvOp::Insert { - collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), - key: b"k".to_vec(), - value: Vec::new(), - ttl_ms: 0, - surrogate: nodedb_types::Surrogate::ZERO, - returning: spec(), - rls_filters: Vec::new(), - }), - PhysicalPlan::Kv(KvOp::InsertIfAbsent { - collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), - key: b"k".to_vec(), - value: Vec::new(), - ttl_ms: 0, - surrogate: nodedb_types::Surrogate::ZERO, - returning: spec(), - rls_filters: Vec::new(), - }), - PhysicalPlan::Kv(KvOp::InsertOnConflictUpdate { - collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), - key: b"k".to_vec(), - value: Vec::new(), - ttl_ms: 0, - updates: Vec::new(), - surrogate: nodedb_types::Surrogate::ZERO, - rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), - returning: spec(), - rls_filters: Vec::new(), - }), - PhysicalPlan::Kv(KvOp::Put { - collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), - key: b"k".to_vec(), - value: Vec::new(), - ttl_ms: 0, - surrogate: nodedb_types::Surrogate::ZERO, - returning: spec(), - rls_filters: Vec::new(), - }), - PhysicalPlan::Kv(KvOp::BatchPut { - collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), - entries: Vec::new(), - ttl_ms: 0, - surrogates: Vec::new(), - returning: spec(), - rls_filters: Vec::new(), - }), - ]; - for plan in &plans { - assert!( - matches!(describe_plan(plan), PlanKind::ReturningRows), - "{plan:?} must shape as rows" - ); - } - } - - /// A plain MERGE reports its affected count under the Postgres `MERGE` tag, - /// not an opaque `OK`. - #[test] - fn merge_without_returning_is_a_dml_result() { - assert!(matches!( - describe_plan(&merge_plan(None)), - PlanKind::DmlResult("MERGE") - )); - } -} diff --git a/nodedb/src/control/server/response_shape/types/plan_kind/array.rs b/nodedb/src/control/server/response_shape/types/plan_kind/array.rs new file mode 100644 index 000000000..23ec59f37 --- /dev/null +++ b/nodedb/src/control/server/response_shape/types/plan_kind/array.rs @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! `ArrayOp` classification. + +use nodedb_physical::physical_plan::ArrayOp; + +use super::kind::PlanKind; + +pub(super) fn describe_array(op: &ArrayOp) -> PlanKind { + match op { + ArrayOp::Slice { .. } => PlanKind::ArraySlice, + + // JSON-array payloads: each row streams as its own pgwire field. + ArrayOp::Project { .. } | ArrayOp::Aggregate { .. } | ArrayOp::Elementwise { .. } => { + PlanKind::MultiRow + } + + // Reports `{"inserted": n}` / `{"deleted": n}`. + ArrayOp::Put { .. } => PlanKind::DmlResult("INSERT"), + ArrayOp::Delete { .. } => PlanKind::DmlResult("DELETE"), + + // Flush/Compact return status JSON — route SingleDocument. + ArrayOp::Flush { .. } | ArrayOp::Compact { .. } => PlanKind::SingleDocument, + + // Array DDL: `{"opened": 1}` / `{"dropped": 1}` status, not a row count. + ArrayOp::OpenArray { .. } + | ArrayOp::DropArray { .. } + | ArrayOp::RestoreArrayDrop { .. } + | ArrayOp::PurgeArrayDrop { .. } + // Internal roaring bitmap for cross-engine prefilter, never a client row. + | ArrayOp::SurrogateBitmapScan { .. } => PlanKind::Execution, + } +} diff --git a/nodedb/src/control/server/response_shape/types/plan_kind/columnar_family.rs b/nodedb/src/control/server/response_shape/types/plan_kind/columnar_family.rs new file mode 100644 index 000000000..64a9cddf7 --- /dev/null +++ b/nodedb/src/control/server/response_shape/types/plan_kind/columnar_family.rs @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! `ColumnarOp`, `TimeseriesOp` and `SpatialOp` classification — the three +//! peer engines on the compressed-column storage core. + +use nodedb_physical::physical_plan::{ColumnarOp, SpatialOp, TimeseriesOp}; + +use super::kind::PlanKind; + +pub(super) fn describe_columnar(op: &ColumnarOp) -> PlanKind { + match op { + ColumnarOp::Scan { .. } => PlanKind::MultiRow, + + ColumnarOp::Insert { + returning: Some(_), .. + } => PlanKind::ReturningRows, + + // Reports `{"accepted": n}`. + ColumnarOp::Insert { .. } => PlanKind::DmlResult("INSERT"), + + // Reports `{"affected": n}`. + ColumnarOp::Update { .. } => PlanKind::DmlResult("UPDATE"), + ColumnarOp::Delete { .. } => PlanKind::DmlResult("DELETE"), + + // Never reach this classifier: write-resolve proposes them and returns + // the response itself, shaped from the intercepted `Update` / `Delete`. + ColumnarOp::ResolvedUpdate { .. } + | ColumnarOp::ResolvedDelete { .. } + // Read-only resolve: payload is the internal row set, never a client row. + | ColumnarOp::ResolveDml { .. } + // Clone materializer payload (`[cursor, entries]`), decoded by its caller. + | ColumnarOp::MaterializeScan { .. } => PlanKind::Execution, + } +} + +pub(super) fn describe_timeseries(op: &TimeseriesOp) -> PlanKind { + match op { + TimeseriesOp::Scan { .. } => PlanKind::MultiRow, + + TimeseriesOp::Ingest { + returning: Some(_), .. + } => PlanKind::ReturningRows, + + // Reports `{"accepted": n}`. + TimeseriesOp::Ingest { .. } => PlanKind::DmlResult("INSERT"), + + // Read-only resolve: payload is the internal admission verdict, never a client row. + TimeseriesOp::ResolveIngest(_) => PlanKind::Execution, + } +} + +pub(super) fn describe_spatial(op: &SpatialOp) -> PlanKind { + match op { + SpatialOp::Scan { .. } => PlanKind::MultiRow, + + // Replication-apply plans (sync inbound, WAL dispatch, Raft apply). + // A client statement never lowers to them, so they answer no client. + SpatialOp::Insert { .. } | SpatialOp::Delete { .. } => PlanKind::Execution, + } +} diff --git a/nodedb/src/control/server/response_shape/types/plan_kind/crdt.rs b/nodedb/src/control/server/response_shape/types/plan_kind/crdt.rs new file mode 100644 index 000000000..8d33cbd31 --- /dev/null +++ b/nodedb/src/control/server/response_shape/types/plan_kind/crdt.rs @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! `CrdtOp` classification. + +use nodedb_physical::physical_plan::CrdtOp; + +use super::kind::PlanKind; + +pub(super) fn describe_crdt(op: &CrdtOp) -> PlanKind { + match op { + CrdtOp::DocUpsert { + returning: Some(_), .. + } + | CrdtOp::DocDelete { + returning: Some(_), .. + } => PlanKind::ReturningRows, + + // INSERT, UPSERT and UPDATE all lower to `DocUpsert`; the verb the + // statement used decides the tag. + CrdtOp::DocUpsert { verb, .. } => PlanKind::DmlResult(verb.command_tag()), + + // A CRDT delete can legitimately remove nothing, so its count must render + // as a DML count from the write's own response, not a document-shaped read. + CrdtOp::DocDelete { .. } => PlanKind::DmlResult("DELETE"), + + // One document body, or one policy object. + CrdtOp::Read { .. } | CrdtOp::ReadAtVersion { .. } | CrdtOp::GetPolicy { .. } => { + PlanKind::SingleDocument + } + + // Delta application and snapshot import: sync/replication writes with + // no row count. + CrdtOp::Apply { .. } + | CrdtOp::ApplyAuthenticated { .. } + | CrdtOp::ImportSnapshot { .. } + // Constraint and policy DDL. + | CrdtOp::SetConstraints { .. } + | CrdtOp::DropConstraints { .. } + | CrdtOp::SetPolicy { .. } + // History maintenance. + | CrdtOp::RestoreToVersion { .. } + | CrdtOp::CompactAtVersion { .. } + // Block-list edits: the handler reports no count. + | CrdtOp::ListInsert { .. } + | CrdtOp::ListDelete { .. } + | CrdtOp::ListMove { .. } + // Internal typed zerompk payloads, decoded by their own dispatcher: + // the installed constraint set, a version vector, a Loro delta, and + // the admission caller's `CrdtPreviewResult`. + | CrdtOp::ReadConstraints { .. } + | CrdtOp::GetVersionVector { .. } + | CrdtOp::ExportDelta { .. } + | CrdtOp::PreviewApply { .. } => PlanKind::Execution, + } +} diff --git a/nodedb/src/control/server/response_shape/types/plan_kind/describe.rs b/nodedb/src/control/server/response_shape/types/plan_kind/describe.rs new file mode 100644 index 000000000..d00ac7ac2 --- /dev/null +++ b/nodedb/src/control/server/response_shape/types/plan_kind/describe.rs @@ -0,0 +1,341 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! `describe_plan`: the one entry point that maps a `PhysicalPlan` to the +//! response shape it produces. Each engine's `*Op` enum is classified in its +//! own file, exhaustively, so a new op is a compile error until it decides. + +use crate::bridge::envelope::PhysicalPlan; + +use super::array::describe_array; +use super::columnar_family::{describe_columnar, describe_spatial, describe_timeseries}; +use super::crdt::describe_crdt; +use super::document::describe_document; +use super::graph::describe_graph; +use super::kind::PlanKind; +use super::kv::describe_kv; +use super::query::describe_query; +use super::search::{describe_text, describe_vector}; + +pub fn describe_plan(plan: &PhysicalPlan) -> PlanKind { + match plan { + PhysicalPlan::Document(op) => describe_document(op), + PhysicalPlan::Kv(op) => describe_kv(op), + PhysicalPlan::Crdt(op) => describe_crdt(op), + PhysicalPlan::Graph(op) => describe_graph(op), + PhysicalPlan::Vector(op) => describe_vector(op), + PhysicalPlan::Text(op) => describe_text(op), + PhysicalPlan::Columnar(op) => describe_columnar(op), + PhysicalPlan::Timeseries(op) => describe_timeseries(op), + PhysicalPlan::Spatial(op) => describe_spatial(op), + PhysicalPlan::Array(op) => describe_array(op), + PhysicalPlan::Query(op) => describe_query(op), + + // Control-plane catalog, session and cluster ops. No `MetaOp` is a + // client DML: none reports a row count, each answers its own caller. + PhysicalPlan::Meta(_) + // Never dispatched through the plan-shaping path: the pgwire cluster + // array router classifies these itself (`routing/cluster_array.rs`). + | PhysicalPlan::ClusterArray(_) + // Event-plane forwarding, answered by its own dispatcher. + | PhysicalPlan::ClusterEvent(_) => PlanKind::Execution, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use nodedb_physical::physical_plan::{ + ArrayOp, CrdtOp, CrdtWriteVerb, DocumentOp, KvOp, TimeseriesOp, + }; + use nodedb_types::{DatabaseId, QualifiedCollection}; + + #[test] + fn crdt_preview_is_an_opaque_execution_plan() { + let plan = PhysicalPlan::Crdt(CrdtOp::PreviewApply { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "tasks"), + document_id: "task-1".to_string(), + delta: vec![0x92, 0x01], + }); + + assert!(matches!(describe_plan(&plan), PlanKind::Execution)); + } + + fn merge_plan( + returning: Option, + ) -> PhysicalPlan { + PhysicalPlan::Document(DocumentOp::Merge { + target_collection: QualifiedCollection::new(DatabaseId::DEFAULT, "target"), + source_collection: QualifiedCollection::new(DatabaseId::DEFAULT, "source"), + source_alias: "s".to_string(), + target_join_col: "id".to_string(), + source_join_col: "id".to_string(), + clauses: Vec::new(), + returning, + resolved_inserts: None, + source_rows: None, + rls_filters: Vec::new(), + rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), + resolved_sum_targets: Vec::new(), + declared_primary_key: None, + }) + } + + /// A `MERGE ... RETURNING` payload is real target rows — `Execution` would + /// pass them unredacted. + #[test] + fn merge_with_returning_is_returning_rows() { + use nodedb_physical::physical_plan::{ReturningColumns, ReturningSpec}; + + let plan = merge_plan(Some(ReturningSpec { + columns: ReturningColumns::Star, + })); + + assert!(matches!(describe_plan(&plan), PlanKind::ReturningRows)); + } + + /// Every insert-family op with a projection must classify row-returning, + /// else it leaks unredacted like the MERGE case above. + #[test] + fn inserts_with_returning_are_returning_rows() { + use nodedb_physical::physical_plan::{ReturningColumns, ReturningSpec}; + + let spec = || { + Some(ReturningSpec { + columns: ReturningColumns::Star, + }) + }; + let plans = [ + PhysicalPlan::Document(DocumentOp::PointInsert { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), + document_id: "d".into(), + value: Vec::new(), + if_absent: false, + surrogate: nodedb_types::Surrogate::ZERO, + returning: spec(), + rls_filters: Vec::new(), + resolved_sum_targets: Vec::new(), + deferred_sum_targets: Vec::new(), + }), + PhysicalPlan::Document(DocumentOp::PointPut { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), + document_id: "d".into(), + value: Vec::new(), + surrogate: nodedb_types::Surrogate::ZERO, + pk_bytes: Vec::new(), + returning: spec(), + rls_filters: Vec::new(), + resolved_sum_targets: Vec::new(), + }), + PhysicalPlan::Document(DocumentOp::BatchInsert { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), + documents: Vec::new(), + surrogates: Vec::new(), + returning: spec(), + rls_filters: Vec::new(), + resolved_sum_targets: Vec::new(), + deferred_sum_targets: Vec::new(), + }), + PhysicalPlan::Document(DocumentOp::Upsert { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), + document_id: "d".into(), + value: Vec::new(), + on_conflict_updates: Vec::new(), + surrogate: nodedb_types::Surrogate::ZERO, + rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), + returning: spec(), + rls_filters: Vec::new(), + resolved_sum_targets: Vec::new(), + }), + ]; + for plan in &plans { + assert!( + matches!(describe_plan(plan), PlanKind::ReturningRows), + "{plan:?} must shape as rows" + ); + } + } + + /// Every KV insert-family op that can carry a projection must classify as + /// row-returning too — the same passthrough leak, one engine over. + #[test] + fn kv_inserts_with_returning_are_returning_rows() { + use nodedb_physical::physical_plan::{ReturningColumns, ReturningSpec}; + + let spec = || { + Some(ReturningSpec { + columns: ReturningColumns::Star, + }) + }; + let plans = [ + PhysicalPlan::Kv(KvOp::Insert { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), + key: b"k".to_vec(), + value: Vec::new(), + ttl_ms: 0, + surrogate: nodedb_types::Surrogate::ZERO, + returning: spec(), + rls_filters: Vec::new(), + }), + PhysicalPlan::Kv(KvOp::InsertIfAbsent { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), + key: b"k".to_vec(), + value: Vec::new(), + ttl_ms: 0, + surrogate: nodedb_types::Surrogate::ZERO, + returning: spec(), + rls_filters: Vec::new(), + }), + PhysicalPlan::Kv(KvOp::InsertOnConflictUpdate { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), + key: b"k".to_vec(), + value: Vec::new(), + ttl_ms: 0, + updates: Vec::new(), + surrogate: nodedb_types::Surrogate::ZERO, + rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), + returning: spec(), + rls_filters: Vec::new(), + }), + PhysicalPlan::Kv(KvOp::Put { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), + key: b"k".to_vec(), + value: Vec::new(), + ttl_ms: 0, + surrogate: nodedb_types::Surrogate::ZERO, + returning: spec(), + rls_filters: Vec::new(), + }), + PhysicalPlan::Kv(KvOp::BatchPut { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), + entries: Vec::new(), + ttl_ms: 0, + surrogates: Vec::new(), + returning: spec(), + rls_filters: Vec::new(), + }), + ]; + for plan in &plans { + assert!( + matches!(describe_plan(plan), PlanKind::ReturningRows), + "{plan:?} must shape as rows" + ); + } + } + + /// A plain MERGE reports its affected count under the Postgres `MERGE` tag, + /// not an opaque `OK`. + #[test] + fn merge_without_returning_is_a_dml_result() { + assert!(matches!( + describe_plan(&merge_plan(None)), + PlanKind::DmlResult("MERGE") + )); + } + + fn kv_put() -> PhysicalPlan { + PhysicalPlan::Kv(KvOp::Put { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), + key: b"k".to_vec(), + value: Vec::new(), + ttl_ms: 0, + surrogate: nodedb_types::Surrogate::ZERO, + returning: None, + rls_filters: Vec::new(), + }) + } + + /// `KvOp::Put` is the SQL `UPSERT` statement: it tags `UPSERT n`, the + /// same as `DocumentOp::Upsert`, the staged path and the Calvin fold. + #[test] + fn kv_put_is_the_upsert_dml_result() { + assert!(matches!( + describe_plan(&kv_put()), + PlanKind::DmlResult("UPSERT") + )); + } + + /// `KvOp::InsertOnConflictUpdate` resolves insert-vs-update at apply time; + /// the tag must follow the verb the handler reports. + #[test] + fn kv_insert_on_conflict_update_is_decided_by_op() { + let plan = PhysicalPlan::Kv(KvOp::InsertOnConflictUpdate { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), + key: b"k".to_vec(), + value: Vec::new(), + ttl_ms: 0, + updates: Vec::new(), + surrogate: nodedb_types::Surrogate::ZERO, + rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), + returning: None, + rls_filters: Vec::new(), + }); + assert!(matches!(describe_plan(&plan), PlanKind::DmlResultByOp)); + } + + /// A timeseries ingest reports `{"accepted": n}` under the `INSERT` tag, + /// not an opaque `OK`. + #[test] + fn timeseries_ingest_is_an_insert_dml_result() { + let plan = PhysicalPlan::Timeseries(TimeseriesOp::Ingest { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "metrics"), + payload: Vec::new(), + format: "ilp".to_string(), + wal_lsn: None, + surrogates: Vec::new(), + provenance: None, + rls_write_check: nodedb_types::RlsWriteCheck::pending_injection(), + returning: None, + rls_filters: Vec::new(), + }); + assert!(matches!( + describe_plan(&plan), + PlanKind::DmlResult("INSERT") + )); + } + + fn crdt_doc_upsert(verb: CrdtWriteVerb) -> PhysicalPlan { + PhysicalPlan::Crdt(CrdtOp::DocUpsert { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "notes"), + document_id: "d1".into(), + fields_json: "{}".into(), + surrogate: nodedb_types::Surrogate::ZERO, + partial: matches!(verb, CrdtWriteVerb::Update), + verb, + returning: None, + rls_filters: Vec::new(), + }) + } + + /// INSERT, UPSERT and UPDATE all lower to `CrdtOp::DocUpsert`; the tag + /// follows the statement verb carried on the op. + #[test] + fn crdt_doc_upsert_tags_by_verb() { + assert!(matches!( + describe_plan(&crdt_doc_upsert(CrdtWriteVerb::Insert)), + PlanKind::DmlResult("INSERT") + )); + assert!(matches!( + describe_plan(&crdt_doc_upsert(CrdtWriteVerb::Upsert)), + PlanKind::DmlResult("UPSERT") + )); + assert!(matches!( + describe_plan(&crdt_doc_upsert(CrdtWriteVerb::Update)), + PlanKind::DmlResult("UPDATE") + )); + } + + /// `INSERT INTO ARRAY` reports `{"inserted": n}` under the `INSERT` tag. + #[test] + fn array_put_is_an_insert_dml_result() { + let plan = PhysicalPlan::Array(ArrayOp::Put { + array_id: nodedb_array::types::ArrayId::new(nodedb_types::TenantId::new(1), "genome"), + cells_msgpack: Vec::new(), + wal_lsn: 0, + provenance: None, + }); + assert!(matches!( + describe_plan(&plan), + PlanKind::DmlResult("INSERT") + )); + } +} diff --git a/nodedb/src/control/server/response_shape/types/plan_kind/document.rs b/nodedb/src/control/server/response_shape/types/plan_kind/document.rs new file mode 100644 index 000000000..963491c8b --- /dev/null +++ b/nodedb/src/control/server/response_shape/types/plan_kind/document.rs @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! `DocumentOp` classification. + +use nodedb_physical::physical_plan::DocumentOp; + +use super::kind::PlanKind; + +pub(super) fn describe_document(op: &DocumentOp) -> PlanKind { + match op { + DocumentOp::PointGet { .. } => PlanKind::SingleDocument, + + DocumentOp::RangeScan { .. } + | DocumentOp::Scan { .. } + | DocumentOp::IndexedFetch { .. } => PlanKind::MultiRow, + + // A write with a projection returns real stored rows and must be + // decoded and redacted, never passed through unshaped. + DocumentOp::PointPut { + returning: Some(_), .. + } + | DocumentOp::PointInsert { + returning: Some(_), .. + } + | DocumentOp::BatchInsert { + returning: Some(_), .. + } + | DocumentOp::PointUpdate { + returning: Some(_), .. + } + | DocumentOp::BulkUpdate { + returning: Some(_), .. + } + | DocumentOp::PointDelete { + returning: Some(_), .. + } + | DocumentOp::BulkDelete { + returning: Some(_), .. + } + | DocumentOp::UpdateFromJoin { + returning: Some(_), .. + } + | DocumentOp::Merge { + returning: Some(_), .. + } + | DocumentOp::Upsert { + returning: Some(_), .. + } => PlanKind::ReturningRows, + + // `PointInsert`: `ON CONFLICT DO NOTHING` makes it no-op-capable, so + // the count must come from the write's response. + DocumentOp::PointPut { .. } + | DocumentOp::PointInsert { .. } + | DocumentOp::BatchInsert { .. } + | DocumentOp::InsertSelect { .. } => PlanKind::DmlResult("INSERT"), + + DocumentOp::PointUpdate { .. } + | DocumentOp::BulkUpdate { .. } + | DocumentOp::UpdateFromJoin { .. } => PlanKind::DmlResult("UPDATE"), + + DocumentOp::PointDelete { .. } | DocumentOp::BulkDelete { .. } => { + PlanKind::DmlResult("DELETE") + } + + // Postgres tags a plain MERGE `MERGE `, matching the staged path. + DocumentOp::Merge { .. } => PlanKind::DmlResult("MERGE"), + + DocumentOp::Truncate { .. } => PlanKind::DmlResult("TRUNCATE"), + + DocumentOp::Upsert { .. } => PlanKind::DmlResult("UPSERT"), + + // Index DDL and catalog maintenance: no row payload, no row count. + DocumentOp::Register { .. } + | DocumentOp::IndexLookup { .. } + | DocumentOp::DropIndex { .. } + | DocumentOp::BackfillIndex { .. } + | DocumentOp::EstimateCount { .. } + // Clone materializer payload (`[cursor, entries]`), decoded by its caller. + | DocumentOp::MaterializeScan { .. } + // Read-only resolve: payload is the internal classification tuple, never a client row. + | DocumentOp::ResolveWrite(_) + // A derived balance write answers no client — reports an affected count only. + | DocumentOp::ApplyBalanceDelta { .. } + // Never reaches this classifier: write-resolve returns the response itself, + // shaped from the intercepted plan whose `returning` slot decides. + | DocumentOp::ResolvedWrite { .. } => PlanKind::Execution, + } +} diff --git a/nodedb/src/control/server/response_shape/types/plan_kind/graph.rs b/nodedb/src/control/server/response_shape/types/plan_kind/graph.rs new file mode 100644 index 000000000..b765a1004 --- /dev/null +++ b/nodedb/src/control/server/response_shape/types/plan_kind/graph.rs @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! `GraphOp` classification. + +use nodedb_physical::physical_plan::GraphOp; + +use super::kind::PlanKind; + +pub(super) fn describe_graph(op: &GraphOp) -> PlanKind { + match op { + // Traversals, pattern matches, algorithms and stats all return one + // row per hit / node / collection. + GraphOp::Hop { .. } + | GraphOp::Neighbors { .. } + | GraphOp::NeighborsMulti { .. } + | GraphOp::Path { .. } + | GraphOp::Subgraph { .. } + | GraphOp::RagFusion { .. } + | GraphOp::Algo { .. } + | GraphOp::Match { .. } + | GraphOp::MatchContinuation { .. } + | GraphOp::MatchVarLenResume { .. } + | GraphOp::BspSuperstep(_) + | GraphOp::WccSuperstep(_) + | GraphOp::TemporalNeighbors { .. } + | GraphOp::TemporalAlgorithm { .. } + | GraphOp::Stats { .. } => PlanKind::MultiRow, + + // Handler reports no count yet. + GraphOp::EdgePut { .. } + | GraphOp::EdgePutBatch { .. } + | GraphOp::EdgeDelete { .. } + | GraphOp::EdgeDeleteBatch { .. } + | GraphOp::SetNodeLabels { .. } + | GraphOp::RemoveNodeLabels { .. } + // Read-only resolve: payload is the internal admission verdict, never a client row. + | GraphOp::ResolveEdgeDelete(_) => PlanKind::Execution, + } +} diff --git a/nodedb/src/control/server/response_shape/types/plan_kind/kind.rs b/nodedb/src/control/server/response_shape/types/plan_kind/kind.rs new file mode 100644 index 000000000..3246839a4 --- /dev/null +++ b/nodedb/src/control/server/response_shape/types/plan_kind/kind.rs @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! The response shape a physical plan produces. +//! +//! Carries no pgwire wire types, so every protocol-specific shaper shares it. + +#[derive(Debug, Clone, Copy)] +pub enum PlanKind { + SingleDocument, + MultiRow, + /// Array slice result — decoded via `ArraySliceResponse` to surface the + /// `truncated_before_horizon` flag as a pgwire NOTICE when set. + ArraySlice, + /// Opaque execution result: DDL, maintenance, an internal stage, or a + /// function-call payload its dispatcher reads directly. pgwire renders a + /// bare `OK` tag. Never a client-facing row-count DML. + Execution, + /// DML operation that returns affected row count. + /// The tag name is used in the pgwire `CommandComplete` message (e.g., "UPDATE", "DELETE"). + DmlResult(&'static str), + /// DML whose verb is decided by the handler at apply time: the payload + /// carries `affected` plus `op` (`"insert"` or `"update"`), read via + /// `extract_kv_conflict_op`. Renders `INSERT 0 n` or `UPDATE n`. + DmlResultByOp, + /// DML with RETURNING clause — payload is a `RowsPayload` (msgpack). + /// Decoded into one pgwire field per column. + ReturningRows, +} diff --git a/nodedb/src/control/server/response_shape/types/plan_kind/kv.rs b/nodedb/src/control/server/response_shape/types/plan_kind/kv.rs new file mode 100644 index 000000000..12a3baf03 --- /dev/null +++ b/nodedb/src/control/server/response_shape/types/plan_kind/kv.rs @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! `KvOp` classification. + +use nodedb_physical::physical_plan::KvOp; + +use super::kind::PlanKind; + +pub(super) fn describe_kv(op: &KvOp) -> PlanKind { + match op { + KvOp::Get { .. } | KvOp::FieldGet { .. } => PlanKind::SingleDocument, + + KvOp::Scan { .. } | KvOp::BatchGet { .. } => PlanKind::MultiRow, + + // A write with a projection returns real stored rows and must be + // decoded and redacted, never passed through unshaped. + KvOp::Insert { + returning: Some(_), .. + } + | KvOp::InsertIfAbsent { + returning: Some(_), .. + } + | KvOp::InsertOnConflictUpdate { + returning: Some(_), .. + } + | KvOp::Put { + returning: Some(_), .. + } + | KvOp::BatchPut { + returning: Some(_), .. + } + | KvOp::FieldSet { + returning: Some(_), .. + } + | KvOp::PredicateUpdate { + returning: Some(_), .. + } + | KvOp::Delete { + returning: Some(_), .. + } + | KvOp::PredicateDelete { + returning: Some(_), .. + } => PlanKind::ReturningRows, + + // The SQL `UPSERT` statement, tagged like `DocumentOp::Upsert`. + KvOp::Put { .. } => PlanKind::DmlResult("UPSERT"), + + // `InsertIfAbsent`: `ON CONFLICT DO NOTHING` makes it no-op-capable, + // so the count must come from the write's response. + KvOp::Insert { .. } | KvOp::InsertIfAbsent { .. } | KvOp::BatchPut { .. } => { + PlanKind::DmlResult("INSERT") + } + + // Insert-vs-update is decided by the handler; the payload says which. + KvOp::InsertOnConflictUpdate { .. } => PlanKind::DmlResultByOp, + + // Reports `{"affected": n}`. `FieldSet` is the keyed UPDATE. + KvOp::FieldSet { .. } | KvOp::PredicateUpdate { .. } => PlanKind::DmlResult("UPDATE"), + + // Counts the keys removed. + KvOp::Delete { .. } | KvOp::PredicateDelete { .. } => PlanKind::DmlResult("DELETE"), + + KvOp::Truncate { .. } => PlanKind::DmlResult("TRUNCATE"), + + // One-object payloads: `{"ttl_ms": n}`, `{"rank": n}`, `{"count": n}`, + // `{"score": ..}`. + KvOp::GetTtl { .. } + | KvOp::SortedIndexRank { .. } + | KvOp::SortedIndexCount { .. } + | KvOp::SortedIndexScore { .. } => PlanKind::SingleDocument, + + // One row per sorted-index entry. + KvOp::SortedIndexTopK { .. } | KvOp::SortedIndexRange { .. } => PlanKind::MultiRow, + + // TTL metadata mutations: no row count. + KvOp::Expire { .. } + | KvOp::Persist { .. } + // Index DDL. + | KvOp::RegisterIndex { .. } + | KvOp::DropIndex { .. } + | KvOp::RegisterSortedIndex { .. } + | KvOp::DropSortedIndex { .. } + // Function-call results (`KV_INCR(..)` and friends): the payload is a + // computed value its dispatcher reads directly, not a row count. + | KvOp::Incr { .. } + | KvOp::IncrFloat { .. } + | KvOp::Cas { .. } + | KvOp::GetSet { .. } + | KvOp::Transfer { .. } + | KvOp::TransferItem { .. } + // Clone materializer payload (`[cursor, entries]`), decoded by its caller. + | KvOp::MaterializeScan { .. } + // Read-only resolve: payload is the internal mutation list, never a client row. + | KvOp::ResolveWrite(_) + // Never reaches this classifier: write-resolve returns the response itself, + // shaped from the intercepted plan whose `returning` slot decides. + | KvOp::ResolvedWrite { .. } => PlanKind::Execution, + } +} diff --git a/nodedb/src/control/server/response_shape/types/plan_kind/mod.rs b/nodedb/src/control/server/response_shape/types/plan_kind/mod.rs new file mode 100644 index 000000000..07757bacf --- /dev/null +++ b/nodedb/src/control/server/response_shape/types/plan_kind/mod.rs @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Protocol-neutral plan classification. +//! +//! One file per engine so every `*Op` enum is matched exhaustively in one +//! place and a new variant fails to compile until it is classified. + +mod array; +mod columnar_family; +mod crdt; +mod describe; +mod document; +mod graph; +mod kind; +mod kv; +mod query; +mod search; + +pub use describe::describe_plan; +pub use kind::PlanKind; diff --git a/nodedb/src/control/server/response_shape/types/plan_kind/query.rs b/nodedb/src/control/server/response_shape/types/plan_kind/query.rs new file mode 100644 index 000000000..23c33e958 --- /dev/null +++ b/nodedb/src/control/server/response_shape/types/plan_kind/query.rs @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! `QueryOp` classification. + +use nodedb_physical::physical_plan::QueryOp; + +use super::describe::describe_plan; +use super::kind::PlanKind; + +pub(super) fn describe_query(op: &QueryOp) -> PlanKind { + match op { + // Exchange means the plan wasn't yet resolved — recurse into the child. + QueryOp::Exchange(exchange) => describe_plan(&exchange.child), + + // PostProcess reshapes a multi-row subquery; its kind is the child's. + QueryOp::PostProcess { input, .. } => describe_plan(input), + + // Constant/catalog-scan expressions compile to ProviderScan, and a + // SetOp resolves to a ProviderScan of merged rows: each element + // streams as its own pgwire row. + QueryOp::ProviderScan { .. } + | QueryOp::SetOp { .. } + | QueryOp::Aggregate { .. } + | QueryOp::FacetCounts { .. } + | QueryOp::HashJoin { .. } + | QueryOp::NestedLoopJoin { .. } + | QueryOp::SortMergeJoin { .. } + | QueryOp::RecursiveScan { .. } + | QueryOp::RecursiveValue { .. } + | QueryOp::LateralTopK { .. } + | QueryOp::LateralLoop { .. } => PlanKind::MultiRow, + + // Intra-plan stages: their payload feeds the next stage of the same + // plan and never reaches a client. + QueryOp::PartialAggregate { .. } + | QueryOp::PartialAggregateState { .. } + | QueryOp::ShuffleJoinConsume { .. } + | QueryOp::ShuffleAggregateConsume { .. } => PlanKind::Execution, + } +} diff --git a/nodedb/src/control/server/response_shape/types/plan_kind/search.rs b/nodedb/src/control/server/response_shape/types/plan_kind/search.rs new file mode 100644 index 000000000..0627c031a --- /dev/null +++ b/nodedb/src/control/server/response_shape/types/plan_kind/search.rs @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! `VectorOp` and `TextOp` classification. + +use nodedb_physical::physical_plan::{TextOp, VectorOp}; + +use super::kind::PlanKind; + +pub(super) fn describe_vector(op: &VectorOp) -> PlanKind { + match op { + VectorOp::DirectUpsert { + returning: Some(_), .. + } => PlanKind::ReturningRows, + // The vector-primary `INSERT`: the handler reports one affected row. + VectorOp::DirectUpsert { .. } => PlanKind::DmlResult("INSERT"), + + VectorOp::Search { .. } + | VectorOp::MultiSearch { .. } + | VectorOp::MultiVectorScoreSearch { .. } + | VectorOp::SparseSearch { .. } => PlanKind::MultiRow, + + // Index-maintenance and config ops: none is a client DML statement, + // and none carries a row payload. Enumerated explicitly so a future + // read op can't silently strand its hits. + VectorOp::Insert { .. } + | VectorOp::BatchInsert { .. } + | VectorOp::Delete { .. } + | VectorOp::DeleteBySurrogate { .. } + | VectorOp::SetParams { .. } + | VectorOp::DropIndex { .. } + | VectorOp::QueryStats { .. } + | VectorOp::Seal { .. } + | VectorOp::CompactIndex { .. } + | VectorOp::Rebuild { .. } + | VectorOp::SparseInsert { .. } + | VectorOp::SparseDelete { .. } + | VectorOp::MultiVectorInsert { .. } + | VectorOp::MultiVectorDelete { .. } => PlanKind::Execution, + } +} + +pub(super) fn describe_text(op: &TextOp) -> PlanKind { + match op { + TextOp::Search { .. } + | TextOp::PhraseSearch { .. } + | TextOp::HybridSearch { .. } + | TextOp::HybridSearchTriple { .. } + | TextOp::BM25ScoreScan { .. } + | TextOp::FtsIndexDoc { .. } + | TextOp::FtsDeleteDoc { .. } => PlanKind::MultiRow, + + // Config write: opaque status. + TextOp::SetTextConfig { .. } => PlanKind::Execution, + } +} diff --git a/nodedb/src/control/server/shared/sql/staging_predicates.rs b/nodedb/src/control/server/shared/sql/staging_predicates.rs index 1f8aea47e..6ab4fab1e 100644 --- a/nodedb/src/control/server/shared/sql/staging_predicates.rs +++ b/nodedb/src/control/server/shared/sql/staging_predicates.rs @@ -131,7 +131,9 @@ pub enum StagedTagKind { KvUpsert { updated: bool, }, - DocUpsert, + /// The SQL `UPSERT` statement (`DocumentOp::Upsert`, `KvOp::Put`): the + /// literal `UPSERT n` tag whatever the insert-vs-update outcome. + Upsert, /// In-transaction `MERGE`, staged as concrete point ops; `affected` is the /// total across arms. pgwire renders `MERGE `. Merge, @@ -156,7 +158,7 @@ pub fn staged_tag_kind(plan: &PhysicalPlan, payload: &[u8]) -> StagedTagKind { PhysicalPlan::Document(DocumentOp::PointDelete { .. } | DocumentOp::BulkDelete { .. }) => { StagedTagKind::Delete } - PhysicalPlan::Document(DocumentOp::Upsert { .. }) => StagedTagKind::DocUpsert, + PhysicalPlan::Document(DocumentOp::Upsert { .. }) => StagedTagKind::Upsert, PhysicalPlan::Kv(op) => staged_kv_tag_kind(op, payload), PhysicalPlan::Columnar(ColumnarOp::Insert { .. }) => StagedTagKind::Insert, // Same Update/Delete tags as the Document bulk predicate-DML arms above. @@ -190,9 +192,10 @@ pub fn staged_tag_kind(plan: &PhysicalPlan, payload: &[u8]) -> StagedTagKind { /// must be a stageable KV write — the enclosing plan already passed [`is_stageable_write`]. fn staged_kv_tag_kind(op: &KvOp, payload: &[u8]) -> StagedTagKind { match op { - KvOp::Put { .. } | KvOp::Insert { .. } | KvOp::InsertIfAbsent { .. } => { - StagedTagKind::Insert - } + // The SQL `UPSERT` statement, tagged like its `DocumentOp::Upsert` + // sibling and like the autocommit `describe_plan` arm. + KvOp::Put { .. } => StagedTagKind::Upsert, + KvOp::Insert { .. } | KvOp::InsertIfAbsent { .. } => StagedTagKind::Insert, KvOp::InsertOnConflictUpdate { .. } => StagedTagKind::KvUpsert { updated: extract_kv_conflict_op(payload).as_deref() == Some("update"), }, @@ -421,6 +424,21 @@ mod tests { } } + #[test] + fn staged_kv_put_is_the_upsert_tag() { + let payload = nodedb_types::json_to_msgpack(&serde_json::json!({ "affected": 1 })).unwrap(); + let op = KvOp::Put { + collection: QualifiedCollection::new(DatabaseId::DEFAULT, "c"), + key: b"k".to_vec(), + value: Vec::new(), + ttl_ms: 0, + surrogate: nodedb_types::Surrogate::ZERO, + returning: None, + rls_filters: Vec::new(), + }; + assert_eq!(staged_kv_tag_kind(&op, &payload), StagedTagKind::Upsert); + } + #[test] fn staged_kv_tag_kind_batch_put_is_insert() { let payload = nodedb_types::json_to_msgpack(&serde_json::json!({ "inserted": 2 })).unwrap(); diff --git a/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs b/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs index 6b83f2606..9f460ddde 100644 --- a/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs +++ b/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs @@ -948,6 +948,7 @@ mod tests { fields_json: "{}".into(), surrogate: Surrogate::ZERO, partial: false, + verb: nodedb_physical::physical_plan::CrdtWriteVerb::Insert, returning: None, rls_filters: Vec::new(), }), diff --git a/nodedb/src/control/server/wal_dispatch/crdt.rs b/nodedb/src/control/server/wal_dispatch/crdt.rs index bbfdd6c99..bc935f5ab 100644 --- a/nodedb/src/control/server/wal_dispatch/crdt.rs +++ b/nodedb/src/control/server/wal_dispatch/crdt.rs @@ -167,6 +167,7 @@ pub(super) fn wal_append_crdt_op( fields_json, surrogate, partial, + verb: _, returning: _, rls_filters: _, } => { @@ -332,6 +333,7 @@ mod tests { fields_json: r#"{"a":1}"#.to_string(), surrogate: Surrogate::new(3), partial: false, + verb: nodedb_physical::physical_plan::CrdtWriteVerb::Insert, returning: None, rls_filters: Vec::new(), }); diff --git a/nodedb/src/control/wal_replication/decode/crdt.rs b/nodedb/src/control/wal_replication/decode/crdt.rs index 156b6fd69..9d9934740 100644 --- a/nodedb/src/control/wal_replication/decode/crdt.rs +++ b/nodedb/src/control/wal_replication/decode/crdt.rs @@ -243,34 +243,9 @@ pub(super) fn list_move( })) } -/// Reconstruct `CrdtOp::DocUpsert` from its wire intent. Unlike the block-list -/// ops, the row's own top-level `surrogate` is carried across the wire and -/// rebuilt via `Surrogate::new` — the live dispatch handler uses it to gate + -/// key the sparse-store materialization. -pub(super) fn doc_upsert( - collection: &str, - document_id: &str, - surrogate: u32, - fields_json: &str, - partial: bool, - returning: Option, - rls_filters: &[u8], -) -> PhysicalPlan { - PhysicalPlan::Crdt(CrdtOp::DocUpsert { - collection: nodedb_types::QualifiedCollection::from_stored(collection.to_owned()), - document_id: document_id.to_owned(), - fields_json: fields_json.to_owned(), - surrogate: nodedb_types::Surrogate::new(surrogate), - partial, - // Carried on the record — a replay re-executes this write for the - // originating request, not just for the follower's own state. - returning, - rls_filters: rls_filters.to_vec(), - }) -} - -/// Reconstruct `CrdtOp::DocDelete` from its wire intent. See [`doc_upsert`] -/// for the surrogate note. +/// Reconstruct `CrdtOp::DocDelete` from its wire intent. The row's own +/// top-level `surrogate` is carried across the wire and rebuilt via +/// `Surrogate::new`. pub(super) fn doc_delete( collection: &str, document_id: &str, @@ -282,7 +257,8 @@ pub(super) fn doc_delete( collection: nodedb_types::QualifiedCollection::from_stored(collection.to_owned()), document_id: document_id.to_owned(), surrogate: nodedb_types::Surrogate::new(surrogate), - // Carried on the record — see `doc_upsert`. + // Carried on the record — a replay re-executes this write for the + // originating request, not just for the follower's own state. returning, rls_filters: rls_filters.to_vec(), }) @@ -313,6 +289,7 @@ mod tests { use crate::control::wal_replication::decode; use crate::control::wal_replication::types::{ReplicatedEntry, ReplicatedWrite}; use crate::types::{DatabaseId, TenantId, VShardId}; + use nodedb_physical::physical_plan::CrdtWriteVerb; use nodedb_types::sync::wire::SyncProvenance; use nodedb_types::{QualifiedCollection, Surrogate}; @@ -684,6 +661,7 @@ mod tests { fields_json: "{}".into(), surrogate: Surrogate::new(4), partial: false, + verb: CrdtWriteVerb::Insert, returning: Some(spec.clone()), rls_filters: b"rls-predicate".to_vec(), }); diff --git a/nodedb/src/control/wal_replication/decode/entry_crdt.rs b/nodedb/src/control/wal_replication/decode/entry_crdt.rs index d483525d6..5ed3a1e3e 100644 --- a/nodedb/src/control/wal_replication/decode/entry_crdt.rs +++ b/nodedb/src/control/wal_replication/decode/entry_crdt.rs @@ -12,6 +12,7 @@ use super::super::types::ReplicatedWrite; use super::crdt; use super::ctx::DecodeCtx; use crate::bridge::envelope::PhysicalPlan; +use nodedb_physical::physical_plan::CrdtOp; pub(super) fn decode_arm(ctx: &DecodeCtx, write: &ReplicatedWrite) -> crate::Result { match write { @@ -153,17 +154,24 @@ pub(super) fn decode_arm(ctx: &DecodeCtx, write: &ReplicatedWrite) -> crate::Res surrogate, fields_json, partial, + verb, returning, rls_filters, - } => Ok(crdt::doc_upsert( - collection, - document_id, - *surrogate, - fields_json, - *partial, - decode_returning(returning)?, - rls_filters, - )), + // The row's own top-level `surrogate` is carried across the wire and + // rebuilt via `Surrogate::new` — the live dispatch handler uses it to + // gate and key the sparse-store materialization. `returning` rides on + // the record so a replay re-executes this write for the originating + // request, not only for the follower's own state. + } => Ok(PhysicalPlan::Crdt(CrdtOp::DocUpsert { + collection: nodedb_types::QualifiedCollection::from_stored(collection.clone()), + document_id: document_id.clone(), + fields_json: fields_json.clone(), + surrogate: nodedb_types::Surrogate::new(*surrogate), + partial: *partial, + verb: *verb, + returning: decode_returning(returning)?, + rls_filters: rls_filters.clone(), + })), ReplicatedWrite::CrdtDocDelete { collection, document_id, diff --git a/nodedb/src/control/wal_replication/encode/crdt.rs b/nodedb/src/control/wal_replication/encode/crdt.rs index 024a4f18d..0707c5ecc 100644 --- a/nodedb/src/control/wal_replication/encode/crdt.rs +++ b/nodedb/src/control/wal_replication/encode/crdt.rs @@ -138,17 +138,19 @@ pub(super) fn encode(op: &CrdtOp) -> Option { fields_json, surrogate, partial, + verb, returning, rls_filters, - } => doc_upsert( - collection.as_str(), - document_id, - surrogate.as_u32(), - fields_json, - *partial, - super::entry::encode_returning(returning), - rls_filters, - ), + } => ReplicatedWrite::CrdtDocUpsert { + collection: collection.as_str().to_owned(), + document_id: document_id.clone(), + surrogate: surrogate.as_u32(), + fields_json: fields_json.clone(), + partial: *partial, + verb: *verb, + returning: super::entry::encode_returning(returning), + rls_filters: rls_filters.clone(), + }, CrdtOp::DocDelete { collection, document_id, @@ -324,26 +326,6 @@ pub(super) fn list_move( } } -pub(super) fn doc_upsert( - collection: &str, - document_id: &str, - surrogate: u32, - fields_json: &str, - partial: bool, - returning: Option>, - rls_filters: &[u8], -) -> ReplicatedWrite { - ReplicatedWrite::CrdtDocUpsert { - collection: collection.to_owned(), - document_id: document_id.to_owned(), - surrogate, - fields_json: fields_json.to_owned(), - partial, - returning, - rls_filters: rls_filters.to_vec(), - } -} - pub(super) fn doc_delete( collection: &str, document_id: &str, diff --git a/nodedb/src/control/wal_replication/types/replicated_write.rs b/nodedb/src/control/wal_replication/types/replicated_write.rs index c12a43dfd..d5495371c 100644 --- a/nodedb/src/control/wal_replication/types/replicated_write.rs +++ b/nodedb/src/control/wal_replication/types/replicated_write.rs @@ -9,7 +9,7 @@ use super::wire_shapes::{ ColumnarResolvedRow, ConstraintChangeOp, DocumentResolvedMutationWire, KvResolvedMutationWire, ReplicatedBatchEdge, ReplicatedSumTarget, }; -use nodedb_physical::physical_plan::{ColumnarInsertIntent, UpdateValue}; +use nodedb_physical::physical_plan::{ColumnarInsertIntent, CrdtWriteVerb, UpdateValue}; use nodedb_types::{PayloadIndexKind, VectorQuantization, VectorStorageDtype}; #[derive( @@ -646,6 +646,8 @@ pub enum ReplicatedWrite { surrogate: u32, fields_json: String, partial: bool, + /// The statement verb, so a decoded plan matches the proposed one. + verb: CrdtWriteVerb, /// See `ReplicatedWrite::PointPut::returning`. #[serde(default)] returning: Option>, diff --git a/nodedb/src/data/executor/core_loop/response.rs b/nodedb/src/data/executor/core_loop/response.rs index e1d50b6c7..787a39be4 100644 --- a/nodedb/src/data/executor/core_loop/response.rs +++ b/nodedb/src/data/executor/core_loop/response.rs @@ -92,10 +92,7 @@ impl CoreLoop { affected: u64, op: &str, ) -> Response { - let mut payload = Vec::with_capacity(32); - nodedb_query::msgpack_scan::write_map_header(&mut payload, 2); - nodedb_query::msgpack_scan::write_kv_i64(&mut payload, "affected", affected as i64); - nodedb_query::msgpack_scan::write_kv_str(&mut payload, "op", op); + let payload = super::super::response_codec::encode_affected_with_op(affected, op); self.response_with_payload(task, payload) } diff --git a/nodedb/src/data/executor/dispatch/crdt.rs b/nodedb/src/data/executor/dispatch/crdt.rs index da8820ad5..361044793 100644 --- a/nodedb/src/data/executor/dispatch/crdt.rs +++ b/nodedb/src/data/executor/dispatch/crdt.rs @@ -209,6 +209,8 @@ impl CoreLoop { fields_json, surrogate, partial, + // Decides the client's command tag only; the write is the same. + verb: _, returning, rls_filters, } => self.execute_crdt_doc_upsert( diff --git a/nodedb/src/data/executor/handlers/kv/resolve/write_ops.rs b/nodedb/src/data/executor/handlers/kv/resolve/write_ops.rs index 39de68e5b..541fa17d2 100644 --- a/nodedb/src/data/executor/handlers/kv/resolve/write_ops.rs +++ b/nodedb/src/data/executor/handlers/kv/resolve/write_ops.rs @@ -86,7 +86,16 @@ impl CoreLoop { let response_payload = match returning { Some(spec) => kv_stored_rows_payload(spec, rls_filters, &[(key, &stored_bytes)])?, - None => Vec::new(), + // Same `{affected, op}` shape `execute_kv_insert_on_conflict_update` + // reports, so the tag renders identically on both paths. + None => response_codec::encode_affected_with_op( + 1, + if existing_bytes.is_some() { + "update" + } else { + "insert" + }, + ), }; Ok(one( diff --git a/nodedb/src/data/executor/response_codec/encode.rs b/nodedb/src/data/executor/response_codec/encode.rs index 25cff1599..daaec53d1 100644 --- a/nodedb/src/data/executor/response_codec/encode.rs +++ b/nodedb/src/data/executor/response_codec/encode.rs @@ -102,7 +102,10 @@ pub(in crate::data::executor) fn encode_value_vec( } /// Encode a simple `{"key": count}` response (for insert confirmations). -pub(in crate::data::executor) fn encode_count(key: &str, count: usize) -> crate::Result> { +/// +/// Crate-visible: the Control-Plane cluster array coordinator emits the same +/// count map a local Data-Plane array write does, so one decoder serves both. +pub(crate) fn encode_count(key: &str, count: usize) -> crate::Result> { let mut map = std::collections::BTreeMap::new(); map.insert(key, count); zerompk::to_msgpack_vec(&map).map_err(|e| crate::Error::Codec { @@ -110,6 +113,17 @@ pub(in crate::data::executor) fn encode_count(key: &str, count: usize) -> crate: }) } +/// Encode `{"affected": n, "op": op}` — the count of a write whose verb the +/// handler decides at apply time (`"insert"` or `"update"`). The Control Plane +/// reads `op` via `extract_kv_conflict_op` to render `INSERT 0 n` / `UPDATE n`. +pub(crate) fn encode_affected_with_op(affected: u64, op: &str) -> Vec { + let mut payload = Vec::with_capacity(32); + nodedb_query::msgpack_scan::write_map_header(&mut payload, 2); + nodedb_query::msgpack_scan::write_kv_i64(&mut payload, "affected", affected as i64); + nodedb_query::msgpack_scan::write_kv_str(&mut payload, "op", op); + payload +} + /// Deserialize a Data-Plane response payload into `T`. /// /// THE counterpart to the encoders above, and the only correct way for a diff --git a/nodedb/src/data/executor/response_codec/mod.rs b/nodedb/src/data/executor/response_codec/mod.rs index 82a38892a..ce2a124c4 100644 --- a/nodedb/src/data/executor/response_codec/mod.rs +++ b/nodedb/src/data/executor/response_codec/mod.rs @@ -33,9 +33,9 @@ pub use arrow::encode_as_arrow_ipc; pub(in crate::data::executor) use decode::decode_response_to_docs; pub use encode::{decode_payload, decode_payload_to_json, decode_payload_value}; pub(in crate::data::executor) use encode::{ - encode, encode_count, encode_json_as_msgpack, encode_json_vec_as_msgpack, encode_serde, - encode_value_vec, + encode, encode_json_as_msgpack, encode_json_vec_as_msgpack, encode_serde, encode_value_vec, }; +pub(crate) use encode::{encode_affected_with_op, encode_count}; #[allow(unused_imports)] pub(crate) use hits::ArrayAggregateResponse; pub(crate) use hits::{ArraySliceResponse, RowsPayload}; diff --git a/nodedb/src/data/executor/wal_replay/crdt_doc.rs b/nodedb/src/data/executor/wal_replay/crdt_doc.rs index 3c0d76ca0..8252517ec 100644 --- a/nodedb/src/data/executor/wal_replay/crdt_doc.rs +++ b/nodedb/src/data/executor/wal_replay/crdt_doc.rs @@ -18,7 +18,7 @@ use crate::bridge::envelope::{PhysicalPlan, Status}; use crate::data::executor::core_loop::CoreLoop; use crate::types::{DatabaseId, Lsn, TenantId, VShardId}; use crate::wal::CrdtDocOpWalRecord; -use nodedb_physical::physical_plan::CrdtOp; +use nodedb_physical::physical_plan::{CrdtOp, CrdtWriteVerb}; use nodedb_types::{RowIdentity, Surrogate}; impl CoreLoop { @@ -99,6 +99,14 @@ impl CoreLoop { fields_json: fields_json.clone(), surrogate, partial, + // Replay answers no client, so the verb only has to match + // the write shape: a partial write is an UPDATE, a full + // replace an INSERT. + verb: if partial { + CrdtWriteVerb::Update + } else { + CrdtWriteVerb::Insert + }, returning: None, rls_filters: Vec::new(), }); @@ -200,7 +208,7 @@ mod tests { use crate::control::server::wal_dispatch::wal_append_if_write; use crate::types::{DatabaseId, TenantId, VShardId}; use crate::wal::manager::WalManager; - use nodedb_physical::physical_plan::CrdtOp; + use nodedb_physical::physical_plan::{CrdtOp, CrdtWriteVerb}; use nodedb_types::{QualifiedCollection, Surrogate}; const TID: u64 = 1; @@ -247,6 +255,11 @@ mod tests { fields_json: fields_json.to_string(), surrogate: Surrogate::new(SURROGATE), partial, + verb: if partial { + CrdtWriteVerb::Update + } else { + CrdtWriteVerb::Insert + }, returning: None, rls_filters: Vec::new(), }) From c052b21b5022fd2e8a1c948eee2e7dcf33435ea9 Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Sat, 19 Sep 2026 09:12:53 +0800 Subject: [PATCH 03/29] refactor(cluster): split Calvin scheduler dispatch by concern Break the static/active transaction dispatch module into active_dispatch, primary_write, and static_dispatch, with primary_write holding the write/RETURNING/change-set classification shared by both dispatch paths. Decide the primary-write participant at transaction level: a slice holding only derived side-effect writes (implicit graph edge, balance delta) deposits an applied response only when the whole transaction has no non-derived write. That keeps a lone balance-delta participant from racing the user's own write for the statement's response, while a Graph-only transaction (the standalone GRAPH INSERT/DELETE EDGE DSL) still deposits the response its caller reads the count from. --- .../driver/core/dispatch/active_dispatch.rs | 160 +++++++++++ .../scheduler/driver/core/dispatch/mod.rs | 7 + .../driver/core/dispatch/primary_write.rs | 101 +++++++ .../static_dispatch.rs} | 248 ++---------------- 4 files changed, 295 insertions(+), 221 deletions(-) create mode 100644 nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/active_dispatch.rs create mode 100644 nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/mod.rs create mode 100644 nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/primary_write.rs rename nodedb/src/control/cluster/calvin/scheduler/driver/core/{dispatch.rs => dispatch/static_dispatch.rs} (54%) diff --git a/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/active_dispatch.rs b/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/active_dispatch.rs new file mode 100644 index 000000000..583934d7b --- /dev/null +++ b/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/active_dispatch.rs @@ -0,0 +1,160 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Active (dependent-read) transaction dispatch: submits a +//! `CalvinExecuteActive` task once all passive results have landed. + +use std::time::Instant; + +use tracing::error; + +use nodedb_cluster::calvin::types::SequencedTxn; +use nodedb_physical::physical_plan::PhysicalPlan; +use nodedb_physical::physical_plan::meta::MetaOp; + +use super::super::scheduler::Scheduler; +use super::primary_write::{ + participant_change_sets, plans_have_primary_write, plans_have_returning, + txn_has_non_derived_write, +}; +use crate::control::cluster::calvin::scheduler::lock_manager::TxnId; + +impl Scheduler { + /// Dispatch an active dependent-read txn once all passive results are in. + pub(in crate::control::cluster::calvin::scheduler::driver::core) fn dispatch_active_txn( + &mut self, + txn: SequencedTxn, + txn_id: TxnId, + lock_owner: TxnId, + injected_reads: std::collections::BTreeMap< + nodedb_physical::physical_plan::meta::PassiveReadKeyId, + nodedb_types::Value, + >, + ) { + let request_id = self.next_request_id(); + let tenant_id = txn.tx_class.tenant_id; + let epoch = txn.epoch; + let position = txn.position; + + let plans = match super::super::super::helpers::decode_plans(&txn.tx_class.plans) { + Ok(p) => p, + Err(e) => { + error!( + vshard_id = self.vshard_id, + epoch, + position, + error = %e, + "calvin scheduler: active plan decode failed; releasing locks" + ); + self.on_txn_complete(txn_id); + return; + } + }; + let has_non_derived_write = txn_has_non_derived_write(&plans); + let plans = match self.local_calvin_plans(plans, txn.tx_class.database_id, epoch, position) + { + Ok(p) if !p.is_empty() => p, + Ok(_) => { + // A dependent-read active txn dispatched here always carries a + // local write slice (the OLLP orchestrator only routes the write + // participant through this path). An empty local slice is a + // routing bug, not a read-only participant — surface it as a + // terminal routing failure rather than dispatching an + // active task with nothing to apply. + let e = crate::Error::Internal { + detail: format!( + "calvin active txn {epoch}/{position} homes no local write plans \ + for vshard {}", + self.vshard_id + ), + }; + error!( + vshard_id = self.vshard_id, + epoch, + position, + error = %e, + "calvin scheduler: active txn homes no local writes; releasing locks" + ); + self.propose_routing_failure(epoch, position, txn_id, &e); + self.on_txn_complete(txn_id); + return; + } + Err(e) => { + error!( + vshard_id = self.vshard_id, + epoch, + position, + error = %e, + "calvin scheduler: active txn routing failed; releasing locks" + ); + self.propose_routing_failure(epoch, position, txn_id, &e); + self.on_txn_complete(txn_id); + return; + } + }; + let has_primary_write = plans_have_primary_write(&plans, has_non_derived_write); + let has_returning = plans_have_returning(&plans); + let change_sets = participant_change_sets(&plans, tenant_id, self.vshard_id); + let plan = PhysicalPlan::Meta(MetaOp::CalvinExecuteActive { + epoch, + position, + tenant_id, + plans, + injected_reads, + epoch_system_ms: txn.epoch_system_ms, + is_group_leader: self.is_group_leader(), + }); + + // Calvin allocates the CalvinApplied WAL LSN post-apply (in the + // scheduler's response handler), so no committed LSN is known at + // dispatch time to stamp here. + let request = + self.build_exempt_request(request_id, tenant_id, txn.tx_class.database_id, plan, None); + + let resp_rx = self.shared.tracker.register(request_id); + + let dispatch_result = match self.shared.dispatcher.lock() { + Ok(mut d) => d.dispatch(request), + Err(poisoned) => poisoned.into_inner().dispatch(request), + }; + + if let Err(e) = dispatch_result { + error!( + vshard_id = self.vshard_id, + epoch, + position, + error = %e, + "calvin scheduler: active dispatch failed; releasing locks" + ); + self.on_txn_complete(txn_id); + return; + } + + self.metrics.record_dispatch(); + + // no-determinism: executor latency observability, off-WAL path + let dispatch_instant = Instant::now(); + + self.spawn_response_bridge(txn_id, request_id, resp_rx); + + self.pending.insert( + txn_id, + super::super::super::types::PendingTxn { + txn, + lock_owner, + // no-determinism: dispatch_time is scheduler observability, not Calvin WAL data + dispatch_time: dispatch_instant, + has_primary_write, + has_returning, + change_sets, + // The dependent-read active path now STAGES (leader-verify OLLP + // + buffer, no base apply); its response drives the same + // resolve → redo → flush as the static path, restoring + // WAL-only-restart durability. `resolve_staged_commit` reads the + // `read_set_valid: None` the active handler returns as "commit". + commit_state: Some(super::super::super::types::CommitState::Staged), + // Set only once the txn parks in `AwaitingVerdict`. + verdict_deadline: None, + }, + ); + } +} diff --git a/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/mod.rs b/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/mod.rs new file mode 100644 index 000000000..d45fca9e9 --- /dev/null +++ b/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/mod.rs @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Static and active (dependent-read) txn dispatch to the Data Plane. + +mod active_dispatch; +mod primary_write; +mod static_dispatch; diff --git a/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/primary_write.rs b/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/primary_write.rs new file mode 100644 index 000000000..d3b8de33d --- /dev/null +++ b/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/primary_write.rs @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Per-slice write classification: primary-write / RETURNING / change-set +//! predicates shared by the static and active dispatch paths. + +use nodedb_physical::physical_plan::PhysicalPlan; + +use crate::types::VShardId; + +/// Whether this vShard's slice carries a PRIMARY user data write — the write +/// whose applied `Response` (affected-count + any RETURNING rows) the +/// coordinator surfaces. +/// +/// A primary write is a Document / KV / Vector / Timeseries / Columnar / Array +/// write — NOT the implicit graph-edge cleanup (`EdgePut` / `EdgeDelete`) that +/// dual-homes alongside a document delete/update. For a single-collection user +/// DML (plus its implicit edges) exactly ONE participant carries the primary +/// write, so only it deposits the applied `Response` into the coordinator's +/// sidecar and the edge participants never clobber the entry. +/// +/// This gate subsumes the RETURNING case (a RETURNING write IS a primary write, +/// so its rows are still deposited) while ALSO carrying the affected-count of a +/// plain (non-RETURNING) write — which a RETURNING-only gate dropped, making a +/// routed plain write report zero rows affected. +pub(super) fn participant_change_sets( + plans: &[PhysicalPlan], + tenant_id: crate::types::TenantId, + vshard_id: u32, +) -> Vec { + plans + .iter() + .filter(|plan| match plan { + // Edge plans are dual-homed; only the source participant publishes + // the one logical Control-Plane event. + PhysicalPlan::Graph( + nodedb_physical::physical_plan::GraphOp::EdgePut { src_id, .. } + | nodedb_physical::physical_plan::GraphOp::EdgeDelete { src_id, .. }, + ) => VShardId::from_key(src_id.as_bytes()).as_u32() == vshard_id, + _ => true, + }) + .map(|plan| { + crate::control::server::dispatch_utils::extract_write_change_set(plan, tenant_id) + }) + .collect() +} + +/// Whether the transaction carries any write that is NOT a derived side +/// effect (an implicit graph edge, a cross-shard balance delta). Decided over +/// the FULL plan set before it is sliced per vShard, because a slice alone +/// cannot tell a lone derived participant from a derived-only statement. +pub(super) fn txn_has_non_derived_write(plans: &[PhysicalPlan]) -> bool { + plans.iter().any(|plan| { + crate::control::planner::calvin::is_write_plan(plan) + && !crate::control::planner::calvin::write_class::is_derived_side_effect(plan) + }) +} + +/// Whether this vShard's slice carries the USER'S own write, as opposed to a +/// derived side effect the Control Plane appended alongside it. +/// +/// It gates the applied-response deposit, and that is the whole reason the +/// distinction has to be made: a statement's `CommandComplete` is shaped from +/// ONE deposited response, primary-write participants coalesce first-wins, and +/// a derived participant's response describes a row the user's statement never +/// named. A balance write that won that race handed an `INSERT` tag a count — +/// or, when its flush found the commit already resolved and answered with an +/// empty payload, no count at all — belonging to a different write entirely. +/// +/// `is_derived_side_effect` is the named predicate rather than an inline +/// `!matches!(plan, PhysicalPlan::Graph(_))`: the implicit graph edge and the +/// cross-shard balance are the same concept, and spelling it inline here is why +/// the second one never inherited the exclusion. +/// +/// `txn_has_non_derived_write` is the transaction-level answer from +/// [`txn_has_non_derived_write`]. When the transaction has one, only the slice +/// holding it is primary. When it has none — the standalone `GRAPH INSERT / +/// DELETE EDGE` DSL's Graph-only tx_class — every write slice is the user's +/// write and deposits; first-wins coalescing then picks one identical count. +/// An empty slice (validate-only read) is never primary. +pub(super) fn plans_have_primary_write( + plans: &[PhysicalPlan], + txn_has_non_derived_write: bool, +) -> bool { + if txn_has_non_derived_write { + return self::txn_has_non_derived_write(plans); + } + plans + .iter() + .any(crate::control::planner::calvin::is_write_plan) +} + +/// Whether this vShard's slice carries a RETURNING-bearing write — a plan whose +/// applied response is DATA-ROWs rather than a bare affected-count. Uses the +/// SAME `describe_plan` classification the coordinator's response-shaping uses, +/// so the two never disagree about which participant owns the returned rows. +pub(super) fn plans_have_returning(plans: &[PhysicalPlan]) -> bool { + use crate::control::server::response_shape::types::{PlanKind, describe_plan}; + plans + .iter() + .any(|plan| matches!(describe_plan(plan), PlanKind::ReturningRows)) +} diff --git a/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch.rs b/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/static_dispatch.rs similarity index 54% rename from nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch.rs rename to nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/static_dispatch.rs index 724cc03dc..6df7d832c 100644 --- a/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch.rs +++ b/nodedb/src/control/cluster/calvin/scheduler/driver/core/dispatch/static_dispatch.rs @@ -1,89 +1,24 @@ // SPDX-License-Identifier: BUSL-1.1 -//! Static and active (dependent-read) txn dispatch to the Data Plane. +//! Static-set ready-transaction dispatch: local-plan routing, group-leader +//! resolution, and the `CalvinExecuteStatic` submit/stage orchestration. use std::time::Instant; use tracing::error; use nodedb_cluster::calvin::types::SequencedTxn; - -use super::routing::PlanRouting; -use super::scheduler::Scheduler; -use crate::control::cluster::calvin::scheduler::lock_manager::TxnId; -use crate::types::{DatabaseId, VShardId}; use nodedb_physical::physical_plan::PhysicalPlan; use nodedb_physical::physical_plan::meta::MetaOp; -/// Whether this vShard's slice carries a PRIMARY user data write — the write -/// whose applied `Response` (affected-count + any RETURNING rows) the -/// coordinator surfaces. -/// -/// A primary write is a Document / KV / Vector / Timeseries / Columnar / Array -/// write — NOT the implicit graph-edge cleanup (`EdgePut` / `EdgeDelete`) that -/// dual-homes alongside a document delete/update. For a single-collection user -/// DML (plus its implicit edges) exactly ONE participant carries the primary -/// write, so only it deposits the applied `Response` into the coordinator's -/// sidecar and the edge participants never clobber the entry. -/// -/// This gate subsumes the RETURNING case (a RETURNING write IS a primary write, -/// so its rows are still deposited) while ALSO carrying the affected-count of a -/// plain (non-RETURNING) write — which a RETURNING-only gate dropped, making a -/// routed plain write report zero rows affected. -fn participant_change_sets( - plans: &[PhysicalPlan], - tenant_id: crate::types::TenantId, - vshard_id: u32, -) -> Vec { - plans - .iter() - .filter(|plan| match plan { - // Edge plans are dual-homed; only the source participant publishes - // the one logical Control-Plane event. - PhysicalPlan::Graph( - nodedb_physical::physical_plan::GraphOp::EdgePut { src_id, .. } - | nodedb_physical::physical_plan::GraphOp::EdgeDelete { src_id, .. }, - ) => VShardId::from_key(src_id.as_bytes()).as_u32() == vshard_id, - _ => true, - }) - .map(|plan| { - crate::control::server::dispatch_utils::extract_write_change_set(plan, tenant_id) - }) - .collect() -} - -/// Whether this vShard's slice carries the USER'S own write, as opposed to a -/// derived side effect the Control Plane appended alongside it. -/// -/// It gates the applied-response deposit, and that is the whole reason the -/// distinction has to be made: a statement's `CommandComplete` is shaped from -/// ONE deposited response, primary-write participants coalesce first-wins, and -/// a derived participant's response describes a row the user's statement never -/// named. A balance write that won that race handed an `INSERT` tag a count — -/// or, when its flush found the commit already resolved and answered with an -/// empty payload, no count at all — belonging to a different write entirely. -/// -/// `is_derived_side_effect` is the named predicate rather than an inline -/// `!matches!(plan, PhysicalPlan::Graph(_))`: the implicit graph edge and the -/// cross-shard balance are the same concept, and spelling it inline here is why -/// the second one never inherited the exclusion. -pub(crate) fn plans_have_primary_write(plans: &[PhysicalPlan]) -> bool { - plans.iter().any(|plan| { - crate::control::planner::calvin::is_write_plan(plan) - && !crate::control::planner::calvin::write_class::is_derived_side_effect(plan) - }) -} - -/// Whether this vShard's slice carries a RETURNING-bearing write — a plan whose -/// applied response is DATA-ROWs rather than a bare affected-count. Uses the -/// SAME `describe_plan` classification the coordinator's response-shaping uses, -/// so the two never disagree about which participant owns the returned rows. -pub(crate) fn plans_have_returning(plans: &[PhysicalPlan]) -> bool { - use crate::control::server::response_shape::types::{PlanKind, describe_plan}; - plans - .iter() - .any(|plan| matches!(describe_plan(plan), PlanKind::ReturningRows)) -} +use super::super::routing::PlanRouting; +use super::super::scheduler::Scheduler; +use super::primary_write::{ + participant_change_sets, plans_have_primary_write, plans_have_returning, + txn_has_non_derived_write, +}; +use crate::control::cluster::calvin::scheduler::lock_manager::TxnId; +use crate::types::DatabaseId; impl Scheduler { /// Whether THIS node is currently the leader of the data-group owning this @@ -115,7 +50,7 @@ impl Scheduler { /// full deadline and report a generic timeout. Mirrors the OllpMismatch /// broadcast in `handle_executor_response`. Shared by `dispatch_txn` and /// `dispatch_active_txn`. - fn propose_routing_failure( + pub(super) fn propose_routing_failure( &self, epoch: u64, position: u32, @@ -151,7 +86,7 @@ impl Scheduler { ) -> crate::Result> { let mut local = Vec::new(); for plan in plans { - match super::routing::plan_vshard_in_database(&plan, database_id) { + match super::super::routing::plan_vshard_in_database(&plan, database_id) { PlanRouting::Vshards(vshards) => { if vshards.iter().any(|v| v.as_u32() == self.vshard_id) { local.push(plan); @@ -203,7 +138,7 @@ impl Scheduler { let epoch = txn.epoch; let position = txn.position; - let plans = match super::super::helpers::decode_plans(&txn.tx_class.plans) { + let plans = match super::super::super::helpers::decode_plans(&txn.tx_class.plans) { Ok(p) => p, Err(e) => { error!( @@ -217,6 +152,7 @@ impl Scheduler { return; } }; + let has_non_derived_write = txn_has_non_derived_write(&plans); let local = match self.local_calvin_plans(plans, txn.tx_class.database_id, epoch, position) { Ok(p) => p, @@ -240,7 +176,7 @@ impl Scheduler { // vote — or a routing bug (neither writes nor reads home here). Only the // latter is an error; the former stages a validate-only task below. if local.is_empty() - && !super::routing::homes_versioned_read( + && !super::super::routing::homes_versioned_read( &txn.tx_class.versioned_reads, txn.tx_class.database_id, self.vshard_id, @@ -270,7 +206,14 @@ impl Scheduler { // identical static path so each casts a real commit/abort Vote through // stage -> resolve -> verdict. The validate-only task stages no plans; // its response carries only the read-set vote. - self.dispatch_calvin_static(txn, txn_id, lock_owner, tenant_id, local); + self.dispatch_calvin_static( + txn, + txn_id, + lock_owner, + tenant_id, + local, + has_non_derived_write, + ); } /// Build and dispatch a `CalvinExecuteStatic` task, then park the txn in @@ -289,6 +232,7 @@ impl Scheduler { lock_owner: TxnId, tenant_id: crate::types::TenantId, plans: Vec, + has_non_derived_write: bool, ) { // The apply-slot identity (used in the CalvinExecuteStatic task and // error logs) is exactly `txn_id`; deriving it here keeps the two in @@ -296,7 +240,7 @@ impl Scheduler { let epoch = txn_id.epoch; let position = txn_id.position; let request_id = self.next_request_id(); - let has_primary_write = plans_have_primary_write(&plans); + let has_primary_write = plans_have_primary_write(&plans, has_non_derived_write); let has_returning = plans_have_returning(&plans); let change_sets = participant_change_sets(&plans, tenant_id, self.vshard_id); let plan = PhysicalPlan::Meta(MetaOp::CalvinExecuteStatic { @@ -346,7 +290,7 @@ impl Scheduler { self.pending.insert( txn_id, - super::super::types::PendingTxn { + super::super::super::types::PendingTxn { txn, lock_owner, // no-determinism: dispatch_time is scheduler observability, not Calvin WAL data @@ -357,145 +301,7 @@ impl Scheduler { // This dispatch STAGED the txn (validate + buffer, no apply); // its response carries the local commit vote that drives the // subsequent flush-or-drop. - commit_state: Some(super::super::types::CommitState::Staged), - // Set only once the txn parks in `AwaitingVerdict`. - verdict_deadline: None, - }, - ); - } - - /// Dispatch an active dependent-read txn once all passive results are in. - pub(in crate::control::cluster::calvin::scheduler::driver::core) fn dispatch_active_txn( - &mut self, - txn: SequencedTxn, - txn_id: TxnId, - lock_owner: TxnId, - injected_reads: std::collections::BTreeMap< - nodedb_physical::physical_plan::meta::PassiveReadKeyId, - nodedb_types::Value, - >, - ) { - let request_id = self.next_request_id(); - let tenant_id = txn.tx_class.tenant_id; - let epoch = txn.epoch; - let position = txn.position; - - let plans = match super::super::helpers::decode_plans(&txn.tx_class.plans) { - Ok(p) => p, - Err(e) => { - error!( - vshard_id = self.vshard_id, - epoch, - position, - error = %e, - "calvin scheduler: active plan decode failed; releasing locks" - ); - self.on_txn_complete(txn_id); - return; - } - }; - let plans = match self.local_calvin_plans(plans, txn.tx_class.database_id, epoch, position) - { - Ok(p) if !p.is_empty() => p, - Ok(_) => { - // A dependent-read active txn dispatched here always carries a - // local write slice (the OLLP orchestrator only routes the write - // participant through this path). An empty local slice is a - // routing bug, not a read-only participant — surface it as a - // terminal routing failure rather than dispatching an - // active task with nothing to apply. - let e = crate::Error::Internal { - detail: format!( - "calvin active txn {epoch}/{position} homes no local write plans \ - for vshard {}", - self.vshard_id - ), - }; - error!( - vshard_id = self.vshard_id, - epoch, - position, - error = %e, - "calvin scheduler: active txn homes no local writes; releasing locks" - ); - self.propose_routing_failure(epoch, position, txn_id, &e); - self.on_txn_complete(txn_id); - return; - } - Err(e) => { - error!( - vshard_id = self.vshard_id, - epoch, - position, - error = %e, - "calvin scheduler: active txn routing failed; releasing locks" - ); - self.propose_routing_failure(epoch, position, txn_id, &e); - self.on_txn_complete(txn_id); - return; - } - }; - let has_primary_write = plans_have_primary_write(&plans); - let has_returning = plans_have_returning(&plans); - let change_sets = participant_change_sets(&plans, tenant_id, self.vshard_id); - let plan = PhysicalPlan::Meta(MetaOp::CalvinExecuteActive { - epoch, - position, - tenant_id, - plans, - injected_reads, - epoch_system_ms: txn.epoch_system_ms, - is_group_leader: self.is_group_leader(), - }); - - // Calvin allocates the CalvinApplied WAL LSN post-apply (in the - // scheduler's response handler), so no committed LSN is known at - // dispatch time to stamp here. - let request = - self.build_exempt_request(request_id, tenant_id, txn.tx_class.database_id, plan, None); - - let resp_rx = self.shared.tracker.register(request_id); - - let dispatch_result = match self.shared.dispatcher.lock() { - Ok(mut d) => d.dispatch(request), - Err(poisoned) => poisoned.into_inner().dispatch(request), - }; - - if let Err(e) = dispatch_result { - error!( - vshard_id = self.vshard_id, - epoch, - position, - error = %e, - "calvin scheduler: active dispatch failed; releasing locks" - ); - self.on_txn_complete(txn_id); - return; - } - - self.metrics.record_dispatch(); - - // no-determinism: executor latency observability, off-WAL path - let dispatch_instant = Instant::now(); - - self.spawn_response_bridge(txn_id, request_id, resp_rx); - - self.pending.insert( - txn_id, - super::super::types::PendingTxn { - txn, - lock_owner, - // no-determinism: dispatch_time is scheduler observability, not Calvin WAL data - dispatch_time: dispatch_instant, - has_primary_write, - has_returning, - change_sets, - // The dependent-read active path now STAGES (leader-verify OLLP - // + buffer, no base apply); its response drives the same - // resolve → redo → flush as the static path, restoring - // WAL-only-restart durability. `resolve_staged_commit` reads the - // `read_set_valid: None` the active handler returns as "commit". - commit_state: Some(super::super::types::CommitState::Staged), + commit_state: Some(super::super::super::types::CommitState::Staged), // Set only once the txn parks in `AwaitingVerdict`. verdict_deadline: None, }, From 027c196270ad66a0bbee68792b93a00ac401d889 Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Sat, 19 Sep 2026 09:13:04 +0800 Subject: [PATCH 04/29] fix(resp): report row counts on graph edge and label writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Edge INSERT/DELETE and node LABEL/UNLABEL previously tagged as Execution/count-less DDL. They now classify as DmlResult (INSERT, DELETE, UPDATE) and carry a real affected count end to end: the Data Plane computes it (existence checks for delete/unlabel, deterministic 1 for put/label), staged writes resolve it against BASE ∪ overlay, the neutral DDL layer threads it through single-home, cross-shard, and in-transaction paths, and the native wire layer surfaces it instead of the 'one command ran' sentinel. Split graph_edge_write.rs into a directory by operation (put, put_batch, delete, delete_batch, shared) to carry the added logic within the file-size limit. --- .../server/native/dispatch/conversion.rs | 22 +- .../response_shape/types/plan_kind/graph.rs | 19 +- .../shared/ddl/neutral/graph_ops/edge.rs | 80 +- .../ddl/neutral/graph_ops/edge_stage.rs | 25 +- nodedb/src/data/executor/dispatch/graph.rs | 26 +- nodedb/src/data/executor/handlers/graph.rs | 2 +- .../executor/handlers/graph_edge_resolve.rs | 4 +- .../executor/handlers/graph_edge_write.rs | 818 ------------------ .../handlers/graph_edge_write/delete.rs | 343 ++++++++ .../handlers/graph_edge_write/delete_batch.rs | 113 +++ .../executor/handlers/graph_edge_write/mod.rs | 15 + .../executor/handlers/graph_edge_write/put.rs | 232 +++++ .../handlers/graph_edge_write/put_batch.rs | 124 +++ .../handlers/graph_edge_write/shared.rs | 151 ++++ .../transaction/overlay/graph_staged/edges.rs | 22 + .../transaction/stage_write/stage_graph.rs | 72 +- nodedb/tests/wire/cases/graph_dsl_handlers.rs | 97 +++ 17 files changed, 1298 insertions(+), 867 deletions(-) delete mode 100644 nodedb/src/data/executor/handlers/graph_edge_write.rs create mode 100644 nodedb/src/data/executor/handlers/graph_edge_write/delete.rs create mode 100644 nodedb/src/data/executor/handlers/graph_edge_write/delete_batch.rs create mode 100644 nodedb/src/data/executor/handlers/graph_edge_write/mod.rs create mode 100644 nodedb/src/data/executor/handlers/graph_edge_write/put.rs create mode 100644 nodedb/src/data/executor/handlers/graph_edge_write/put_batch.rs create mode 100644 nodedb/src/data/executor/handlers/graph_edge_write/shared.rs diff --git a/nodedb/src/control/server/native/dispatch/conversion.rs b/nodedb/src/control/server/native/dispatch/conversion.rs index 8dc8d0ded..390d7b1bd 100644 --- a/nodedb/src/control/server/native/dispatch/conversion.rs +++ b/nodedb/src/control/server/native/dispatch/conversion.rs @@ -166,7 +166,20 @@ pub(crate) fn ddl_result_to_native( // the first element is the first meaningful result — mirroring the // previous bridge, which returned on the first known variant. Ok(results) => match results.into_iter().next() { - Some(DdlResult::Status { command, .. }) => NativeResponse::status_row(seq, command), + Some(DdlResult::Status { + command, + rows_affected, + }) => { + let mut r = NativeResponse::status_row(seq, command); + // `status_row` defaults to the `Some(1)` "one command ran" + // sentinel for count-less DDL. A count-bearing status (the + // graph edge/label DSL statements) overrides it with the + // real affected count instead. + if let Some(n) = rows_affected { + r.rows_affected = Some(n); + } + r + } Some(DdlResult::Rows(shaped)) => { let (columns, rows) = to_native_columns_rows(&shaped); NativeResponse { @@ -258,9 +271,10 @@ pub(crate) fn calvin_native_response( if let Some(resp) = &apply_result { r.watermark_lsn = resp.watermark_lsn.as_u64(); } - // A batch with no count-bearing plan (pure graph / vector / DDL work) has no - // row count to report, and says so by leaving `rows_affected` unset rather - // than inventing one from the task count. + // A batch with no count-bearing plan (vector / DDL work) has no row count + // to report, and says so by leaving `rows_affected` unset rather than + // inventing one from the task count. Graph edge/label writes classify as + // count-bearing (`PlanKind::DmlResult`), same as any other DML. if dml_plan.is_some() { let count = apply_result.as_ref().map_or_else( || { diff --git a/nodedb/src/control/server/response_shape/types/plan_kind/graph.rs b/nodedb/src/control/server/response_shape/types/plan_kind/graph.rs index b765a1004..10489a26a 100644 --- a/nodedb/src/control/server/response_shape/types/plan_kind/graph.rs +++ b/nodedb/src/control/server/response_shape/types/plan_kind/graph.rs @@ -26,14 +26,17 @@ pub(super) fn describe_graph(op: &GraphOp) -> PlanKind { | GraphOp::TemporalAlgorithm { .. } | GraphOp::Stats { .. } => PlanKind::MultiRow, - // Handler reports no count yet. - GraphOp::EdgePut { .. } - | GraphOp::EdgePutBatch { .. } - | GraphOp::EdgeDelete { .. } + GraphOp::EdgePut { .. } | GraphOp::EdgePutBatch { .. } => PlanKind::DmlResult("INSERT"), + + // `ResolveEdgeDelete` reports the same live/absent verdict as the + // delete it wraps, via `response_affected` — matches an edge delete's + // tag even though the resolve pass itself writes nothing. + GraphOp::EdgeDelete { .. } | GraphOp::EdgeDeleteBatch { .. } - | GraphOp::SetNodeLabels { .. } - | GraphOp::RemoveNodeLabels { .. } - // Read-only resolve: payload is the internal admission verdict, never a client row. - | GraphOp::ResolveEdgeDelete(_) => PlanKind::Execution, + | GraphOp::ResolveEdgeDelete(_) => PlanKind::DmlResult("DELETE"), + + GraphOp::SetNodeLabels { .. } | GraphOp::RemoveNodeLabels { .. } => { + PlanKind::DmlResult("UPDATE") + } } } diff --git a/nodedb/src/control/server/shared/ddl/neutral/graph_ops/edge.rs b/nodedb/src/control/server/shared/ddl/neutral/graph_ops/edge.rs index 955583ee8..23e63430d 100644 --- a/nodedb/src/control/server/shared/ddl/neutral/graph_ops/edge.rs +++ b/nodedb/src/control/server/shared/ddl/neutral/graph_ops/edge.rs @@ -12,6 +12,7 @@ use crate::bridge::envelope::PhysicalPlan; use crate::control::planner::calvin::{build_static_tx_class, submit_calvin_routed}; use crate::control::security::identity::AuthenticatedIdentity; use crate::control::server::shared::session::{DmlTxnCtx, TransactionState}; +use crate::control::server::shared::sql::staging_predicates::require_affected_count; use crate::control::server::surrogate_exchange::assign_surrogate_routed; use crate::control::state::SharedState; use crate::types::{DatabaseId, TraceId, VShardId}; @@ -22,6 +23,17 @@ use super::super::super::result::{DdlError, DdlResult}; use super::edge_parse::{properties_to_json, validate_edge_label}; use super::support::{data_plane_verdict, ddl_err}; +/// Read the affected count off a Data-Plane response, mapping a missing count +/// to a [`DdlError`] via `ddl_err` — never a default. +fn response_affected(response: &crate::bridge::envelope::Response) -> Result { + require_affected_count(response.payload.as_bytes()).map_err(|e| { + ddl_err( + "XX000", + format!("edge write response is missing its affected count: {e}"), + ) + }) +} + /// `GRAPH INSERT EDGE IN '' FROM '' TO '' TYPE '