Skip to content

Commit b6a2052

Browse files
committed
feat(crdt): support RETURNING on CRDT document UPDATE/DELETE
Adds a response-only ReturningSpec field to CrdtOp::DocUpsert and DocDelete, threaded through the planner (UPDATE/DELETE now route to DocUpsert/DocDelete with RETURNING instead of being rejected), the pgwire returning-spec injection, response-shape classification, WAL dispatch/replication, and the Data Plane handlers that project the written or pre-deletion row. WAL and replicated records still encode with returning=None since the projection is response-only.
1 parent d600433 commit b6a2052

15 files changed

Lines changed: 501 additions & 48 deletions

File tree

nodedb-physical/src/physical_plan/crdt.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
55
use nodedb_types::Surrogate;
66

7+
use super::ReturningSpec;
8+
79
/// CRDT engine physical operations.
810
#[derive(
911
Debug,
@@ -199,12 +201,20 @@ pub enum CrdtOp {
199201
fields_json: String,
200202
surrogate: Surrogate,
201203
partial: bool,
204+
/// Response-only RETURNING projection; None for a plain write. NOT
205+
/// persisted/replicated — WAL/replication reconstruct with None.
206+
#[serde(default)]
207+
returning: Option<ReturningSpec>,
202208
},
203209
/// Delete a document row: tombstone in the collection's Loro doc + remove
204210
/// from the sparse document store.
205211
DocDelete {
206212
collection: String,
207213
document_id: String,
208214
surrogate: Surrogate,
215+
/// Response-only RETURNING projection; None for a plain write. NOT
216+
/// persisted/replicated — WAL/replication reconstruct with None.
217+
#[serde(default)]
218+
returning: Option<ReturningSpec>,
209219
},
210220
}

nodedb/src/control/planner/sql_plan_convert/dml/insert.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -272,6 +272,7 @@ pub(in super::super) fn convert_insert(
272272
fields_json: super::crdt_gate::row_to_fields_json(row)?,
273273
surrogate,
274274
partial: false,
275+
returning: None,
275276
})
276277
} else {
277278
PhysicalPlan::Document(DocumentOp::PointInsert {
@@ -413,6 +414,7 @@ pub(in super::super) fn convert_upsert(
413414
fields_json: super::crdt_gate::row_to_fields_json(row)?,
414415
surrogate,
415416
partial: false,
417+
returning: None,
416418
})
417419
} else {
418420
PhysicalPlan::Document(DocumentOp::Upsert {

nodedb/src/control/planner/sql_plan_convert/dml/update_delete/delete.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_delete(
130130
collection: collection.into(),
131131
document_id: pk_string,
132132
surrogate,
133+
returning: None,
133134
})
134135
} else {
135136
PhysicalPlan::Document(DocumentOp::PointDelete {

nodedb/src/control/planner/sql_plan_convert/dml/update_delete/tests.rs

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -293,16 +293,34 @@ fn non_literal_rhs_update_on_crdt_rejects() {
293293
}
294294

295295
#[test]
296-
fn update_returning_on_crdt_rejects() {
296+
fn update_returning_on_crdt_routes_to_doc_upsert() {
297+
// RETURNING is stripped before planning and re-injected downstream
298+
// (pgwire `inject_returning_spec` attaches the spec to the CrdtOp, and the
299+
// DP handler emits the projected rows). So at the convert layer a RETURNING
300+
// UPDATE on a crdt collection routes to `DocUpsert` exactly like a plain
301+
// UPDATE — it is NOT rejected. `returning` on the op is `None` here; the
302+
// spec is attached later at the protocol boundary.
297303
let (ctx, _dir) = ctx_with_catalog();
298304
let assignments = vec![(
299305
"name".to_string(),
300306
SqlExpr::Literal(SqlValue::String("bob".to_string())),
301307
)];
302308
let keys = vec![SqlValue::String("k1".to_string())];
303-
let err = convert_update(update_params("crdt_coll", &assignments, &keys, true, &ctx))
304-
.expect_err("UPDATE ... RETURNING on crdt must reject");
305-
assert!(matches!(err, crate::Error::BadRequest { .. }));
309+
let tasks = convert_update(update_params("crdt_coll", &assignments, &keys, true, &ctx))
310+
.expect("UPDATE ... RETURNING on crdt must route, not reject");
311+
assert_eq!(tasks.len(), 1);
312+
match &tasks[0].plan {
313+
PhysicalPlan::Crdt(CrdtOp::DocUpsert {
314+
partial, returning, ..
315+
}) => {
316+
assert!(partial, "UPDATE SET must be a partial DocUpsert");
317+
assert!(
318+
returning.is_none(),
319+
"RETURNING spec is attached downstream, not at convert"
320+
);
321+
}
322+
other => panic!("expected partial CrdtOp::DocUpsert, got {other:?}"),
323+
}
306324
}
307325

308326
#[test]

nodedb/src/control/planner/sql_plan_convert/dml/update_delete/update.rs

Lines changed: 15 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_update(
4141
assignments,
4242
filters,
4343
target_keys,
44-
returning,
44+
returning: _returning,
4545
tenant_id,
4646
ctx,
4747
} = params;
@@ -146,26 +146,20 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_update(
146146

147147
// CRDT GATE: an UPDATE on a `crdt = true` document collection routes to
148148
// `CrdtOp::DocUpsert` with `partial = true` (LWW-per-field). Only PK-targeted
149-
// SET with literal RHS is representable. Predicate (non-PK) UPDATE, non-literal
150-
// RHS, and UPDATE ... RETURNING are rejected — there is NO silent fallthrough
151-
// to a `DocumentOp`, which would bypass CRDT convergence. Read the flag ONCE.
149+
// SET with literal RHS is representable. Predicate (non-PK) UPDATE and
150+
// non-literal RHS are rejected — there is NO silent fallthrough to a
151+
// `DocumentOp`, which would bypass CRDT convergence. `RETURNING` IS
152+
// supported: the response-only projection is injected into the plan later
153+
// and emitted by the Data Plane handler, exactly like `PointUpdate`. Read
154+
// the flag ONCE.
152155
let is_crdt = super::super::crdt_gate::document_collection_is_crdt(ctx, collection)?;
153-
if is_crdt {
154-
if returning {
155-
return Err(crate::Error::BadRequest {
156-
detail: format!(
157-
"UPDATE ... RETURNING on CRDT collection '{collection}' is not supported"
158-
),
159-
});
160-
}
161-
if target_keys.is_empty() {
162-
return Err(crate::Error::BadRequest {
163-
detail: format!(
164-
"predicate (non-primary-key) UPDATE on CRDT collection '{collection}' is not \
165-
supported; target rows by primary key"
166-
),
167-
});
168-
}
156+
if is_crdt && target_keys.is_empty() {
157+
return Err(crate::Error::BadRequest {
158+
detail: format!(
159+
"predicate (non-primary-key) UPDATE on CRDT collection '{collection}' is not \
160+
supported; target rows by primary key"
161+
),
162+
});
169163
}
170164
// Partial-update payload for the CRDT path, built ONCE from the literal SET
171165
// assignments (non-literal RHS rejected inside the builder).
@@ -246,6 +240,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_update(
246240
fields_json: fields_json.clone(),
247241
surrogate,
248242
partial: true,
243+
returning: None,
249244
})
250245
} else {
251246
PhysicalPlan::Document(DocumentOp::PointUpdate {

nodedb/src/control/server/pgwire/handler/routing/planning.rs

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -257,14 +257,15 @@ pub(super) fn consistency_for_tasks(tasks: &[PhysicalTask]) -> crate::types::Rea
257257

258258
/// Inject a RETURNING spec into a DML physical plan variant.
259259
///
260-
/// Only `PointUpdate`, `BulkUpdate`, `PointDelete`, and `BulkDelete` are
261-
/// affected. All other plan variants are left unchanged.
260+
/// Only `PointUpdate`, `BulkUpdate`, `PointDelete`, `BulkDelete`,
261+
/// `UpdateFromJoin`, and the CRDT `DocUpsert` / `DocDelete` ops are affected.
262+
/// All other plan variants are left unchanged.
262263
pub(super) fn inject_returning_spec(
263264
plan: &mut crate::bridge::envelope::PhysicalPlan,
264265
spec: nodedb_physical::physical_plan::ReturningSpec,
265266
) {
266267
use crate::bridge::envelope::PhysicalPlan;
267-
use nodedb_physical::physical_plan::DocumentOp;
268+
use nodedb_physical::physical_plan::{CrdtOp, DocumentOp};
268269

269270
match plan {
270271
PhysicalPlan::Document(DocumentOp::PointUpdate { returning, .. }) => {
@@ -282,6 +283,12 @@ pub(super) fn inject_returning_spec(
282283
PhysicalPlan::Document(DocumentOp::UpdateFromJoin { returning, .. }) => {
283284
*returning = Some(spec);
284285
}
286+
PhysicalPlan::Crdt(CrdtOp::DocUpsert { returning, .. }) => {
287+
*returning = Some(spec);
288+
}
289+
PhysicalPlan::Crdt(CrdtOp::DocDelete { returning, .. }) => {
290+
*returning = Some(spec);
291+
}
285292
_ => {}
286293
}
287294
}

nodedb/src/control/server/response_shape/types.rs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,13 @@ pub enum PlanKind {
2929

3030
pub fn describe_plan(plan: &PhysicalPlan) -> PlanKind {
3131
match plan {
32+
PhysicalPlan::Crdt(CrdtOp::DocUpsert {
33+
returning: Some(_), ..
34+
})
35+
| PhysicalPlan::Crdt(CrdtOp::DocDelete {
36+
returning: Some(_), ..
37+
}) => PlanKind::ReturningRows,
38+
3239
PhysicalPlan::Document(DocumentOp::PointGet { .. })
3340
| PhysicalPlan::Crdt(CrdtOp::Read { .. })
3441
| PhysicalPlan::Crdt(CrdtOp::GetPolicy { .. })
@@ -167,7 +174,8 @@ pub fn describe_plan(plan: &PhysicalPlan) -> PlanKind {
167174

168175
// Default: opaque execution result. The specific arms above take
169176
// precedence; these inner wildcards catch every unmatched op of each
170-
// engine plus the engines with no arms here (Crdt, Meta, ClusterArray).
177+
// engine (including the remaining `Crdt` ops not covered above) plus
178+
// the engines with no arms at all here (Meta, ClusterArray).
171179
// Exhaustive so a new PhysicalPlan variant forces a decision.
172180
PhysicalPlan::Document(_)
173181
| PhysicalPlan::Graph(_)

nodedb/src/control/server/shared/write_admission/predicate/txn_buffering.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1003,11 +1003,13 @@ mod tests {
10031003
fields_json: "{}".into(),
10041004
surrogate: Surrogate::ZERO,
10051005
partial: false,
1006+
returning: None,
10061007
}),
10071008
PhysicalPlan::Crdt(CrdtOp::DocDelete {
10081009
collection: "c".into(),
10091010
document_id: "d".into(),
10101011
surrogate: Surrogate::ZERO,
1012+
returning: None,
10111013
}),
10121014
];
10131015
for p in &plans {

nodedb/src/control/server/wal_dispatch/crdt.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ pub(super) fn wal_append_crdt_op(
125125
fields_json,
126126
surrogate,
127127
partial,
128+
returning: _,
128129
} => {
129130
// Intent-logged like the block-list ops: the Data Plane builds the
130131
// Loro mutation and the Control Plane has no `LoroDoc`, so the
@@ -144,6 +145,7 @@ pub(super) fn wal_append_crdt_op(
144145
collection,
145146
document_id,
146147
surrogate,
148+
returning: _,
147149
} => {
148150
let payload = crate::wal::CrdtDocOpWalRecord::Delete {
149151
collection: collection.clone(),
@@ -281,6 +283,7 @@ mod tests {
281283
fields_json: r#"{"a":1}"#.to_string(),
282284
surrogate: Surrogate::new(3),
283285
partial: false,
286+
returning: None,
284287
});
285288

286289
let outcome = super::super::wal_append_if_write(

nodedb/src/control/wal_replication/decode/crdt.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,7 @@ pub(super) fn doc_upsert(
131131
fields_json: fields_json.to_owned(),
132132
surrogate: nodedb_types::Surrogate::new(surrogate),
133133
partial,
134+
returning: None,
134135
})
135136
}
136137

@@ -141,6 +142,7 @@ pub(super) fn doc_delete(collection: &str, document_id: &str, surrogate: u32) ->
141142
collection: collection.to_owned(),
142143
document_id: document_id.to_owned(),
143144
surrogate: nodedb_types::Surrogate::new(surrogate),
145+
returning: None,
144146
})
145147
}
146148

0 commit comments

Comments
 (0)