Skip to content

Commit 134cce1

Browse files
Engine: duplicate INSERT on unique identity returns success as no-op
1 parent 9420692 commit 134cce1

5 files changed

Lines changed: 181 additions & 89 deletions

File tree

crates/contextdb-engine/src/database.rs

Lines changed: 119 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,16 @@ pub struct Database {
8282
closed: AtomicBool,
8383
}
8484

85+
pub(crate) enum InsertRowResult {
86+
Inserted(RowId),
87+
NoOp,
88+
}
89+
90+
enum RowConstraintCheck {
91+
Valid,
92+
DuplicateUniqueNoOp,
93+
}
94+
8595
impl std::fmt::Debug for Database {
8696
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
8797
f.debug_struct("Database")
@@ -768,6 +778,21 @@ impl Database {
768778
self.relational.insert(tx, table, values)
769779
}
770780

781+
pub(crate) fn insert_row_with_unique_noop(
782+
&self,
783+
tx: TxId,
784+
table: &str,
785+
values: HashMap<ColName, Value>,
786+
) -> Result<InsertRowResult> {
787+
match self.check_row_constraints(tx, table, &values, None, true)? {
788+
RowConstraintCheck::Valid => self
789+
.relational
790+
.insert(tx, table, values)
791+
.map(InsertRowResult::Inserted),
792+
RowConstraintCheck::DuplicateUniqueNoOp => Ok(InsertRowResult::NoOp),
793+
}
794+
}
795+
771796
pub fn upsert_row(
772797
&self,
773798
tx: TxId,
@@ -816,6 +841,22 @@ impl Database {
816841
values: &HashMap<ColName, Value>,
817842
skip_row_id: Option<RowId>,
818843
) -> Result<()> {
844+
match self.check_row_constraints(tx, table, values, skip_row_id, false)? {
845+
RowConstraintCheck::Valid => Ok(()),
846+
RowConstraintCheck::DuplicateUniqueNoOp => {
847+
unreachable!("strict constraint validation cannot return no-op")
848+
}
849+
}
850+
}
851+
852+
fn check_row_constraints(
853+
&self,
854+
tx: TxId,
855+
table: &str,
856+
values: &HashMap<ColName, Value>,
857+
skip_row_id: Option<RowId>,
858+
allow_duplicate_unique_noop: bool,
859+
) -> Result<RowConstraintCheck> {
819860
let meta = self
820861
.table_meta(table)
821862
.ok_or_else(|| Error::TableNotFound(table.to_string()))?;
@@ -827,11 +868,7 @@ impl Database {
827868
skip_row_id.is_none_or(|row_id| row.row_id != row_id)
828869
})?;
829870

830-
for column in meta
831-
.columns
832-
.iter()
833-
.filter(|column| column.unique && !column.primary_key)
834-
{
871+
for column in meta.columns.iter().filter(|column| column.primary_key) {
835872
let Some(value) = values.get(&column.name) else {
836873
continue;
837874
};
@@ -849,6 +886,33 @@ impl Database {
849886
}
850887
}
851888

889+
let mut duplicate_unique_row_id = None;
890+
891+
for column in meta
892+
.columns
893+
.iter()
894+
.filter(|column| column.unique && !column.primary_key)
895+
{
896+
let Some(value) = values.get(&column.name) else {
897+
continue;
898+
};
899+
if *value == Value::Null {
900+
continue;
901+
}
902+
let matching_row_ids: Vec<RowId> = visible_rows
903+
.iter()
904+
.filter(|existing| existing.values.get(&column.name) == Some(value))
905+
.map(|existing| existing.row_id)
906+
.collect();
907+
self.merge_unique_conflict(
908+
table,
909+
&column.name,
910+
&matching_row_ids,
911+
allow_duplicate_unique_noop,
912+
&mut duplicate_unique_row_id,
913+
)?;
914+
}
915+
852916
for unique_constraint in &meta.unique_constraints {
853917
let mut candidate_values = Vec::with_capacity(unique_constraint.len());
854918
let mut has_null = false;
@@ -867,17 +931,60 @@ impl Database {
867931
continue;
868932
}
869933

870-
if visible_rows.iter().any(|existing| {
871-
unique_constraint
872-
.iter()
873-
.zip(candidate_values.iter())
874-
.all(|(column_name, value)| existing.values.get(column_name) == Some(*value))
875-
}) {
934+
let matching_row_ids: Vec<RowId> = visible_rows
935+
.iter()
936+
.filter(|existing| {
937+
unique_constraint.iter().zip(candidate_values.iter()).all(
938+
|(column_name, value)| existing.values.get(column_name) == Some(*value),
939+
)
940+
})
941+
.map(|existing| existing.row_id)
942+
.collect();
943+
self.merge_unique_conflict(
944+
table,
945+
&unique_constraint.join(","),
946+
&matching_row_ids,
947+
allow_duplicate_unique_noop,
948+
&mut duplicate_unique_row_id,
949+
)?;
950+
}
951+
952+
if duplicate_unique_row_id.is_some() {
953+
Ok(RowConstraintCheck::DuplicateUniqueNoOp)
954+
} else {
955+
Ok(RowConstraintCheck::Valid)
956+
}
957+
}
958+
959+
fn merge_unique_conflict(
960+
&self,
961+
table: &str,
962+
column: &str,
963+
matching_row_ids: &[RowId],
964+
allow_duplicate_unique_noop: bool,
965+
duplicate_unique_row_id: &mut Option<RowId>,
966+
) -> Result<()> {
967+
if matching_row_ids.is_empty() {
968+
return Ok(());
969+
}
970+
971+
if !allow_duplicate_unique_noop || matching_row_ids.len() != 1 {
972+
return Err(Error::UniqueViolation {
973+
table: table.to_string(),
974+
column: column.to_string(),
975+
});
976+
}
977+
978+
let matched_row_id = matching_row_ids[0];
979+
if let Some(existing_row_id) = duplicate_unique_row_id {
980+
if *existing_row_id != matched_row_id {
876981
return Err(Error::UniqueViolation {
877982
table: table.to_string(),
878-
column: unique_constraint.join(","),
983+
column: column.to_string(),
879984
});
880985
}
986+
} else {
987+
*duplicate_unique_row_id = Some(matched_row_id);
881988
}
882989

883990
Ok(())

crates/contextdb-engine/src/executor.rs

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use crate::database::{Database, QueryResult};
1+
use crate::database::{Database, InsertRowResult, QueryResult};
22
use crate::sync_types::ConflictPolicy;
33
use contextdb_core::*;
44
use contextdb_parser::ast::{
@@ -986,8 +986,12 @@ fn exec_insert(
986986
}
987987
}
988988
} else {
989-
match db.insert_row(txid, &p.table, values.clone()) {
990-
Ok(row_id) => row_id,
989+
match db.insert_row_with_unique_noop(txid, &p.table, values.clone()) {
990+
Ok(InsertRowResult::Inserted(row_id)) => row_id,
991+
Ok(InsertRowResult::NoOp) => {
992+
db.accountant().release(row_bytes);
993+
continue;
994+
}
991995
Err(err) => {
992996
db.accountant().release(row_bytes);
993997
return Err(err);

crates/contextdb-engine/src/schema_enforcer.rs

Lines changed: 0 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -93,40 +93,6 @@ pub fn validate_dml(
9393
}
9494
}
9595

96-
let unique_columns: Vec<_> = table_meta
97-
.columns
98-
.iter()
99-
.filter(|column| column.unique && !column.primary_key)
100-
.collect();
101-
if !unique_columns.is_empty() {
102-
let existing_rows = db.scan(&p.table, db.snapshot())?;
103-
for row in &p.values {
104-
for column in &unique_columns {
105-
let Some(index) = columns.iter().position(|name| name == &column.name)
106-
else {
107-
continue;
108-
};
109-
let value = row
110-
.get(index)
111-
.ok_or_else(|| {
112-
Error::PlanError("column/value count mismatch".to_string())
113-
})
114-
.and_then(|expr| resolve_expr(expr, params))?;
115-
if value == Value::Null {
116-
continue;
117-
}
118-
if existing_rows
119-
.iter()
120-
.any(|existing| existing.values.get(&column.name) == Some(&value))
121-
{
122-
return Err(Error::UniqueViolation {
123-
table: p.table.clone(),
124-
column: column.name.clone(),
125-
});
126-
}
127-
}
128-
}
129-
}
13096
Ok(())
13197
}
13298
_ => Ok(()),

crates/contextdb-engine/tests/sql_surface_tests.rs

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1848,7 +1848,7 @@ fn con_01_not_null_rejects_null() {
18481848
}
18491849

18501850
#[test]
1851-
fn con_02_unique_rejects_duplicate() {
1851+
fn con_02_unique_duplicate_is_noop() {
18521852
let db = Database::open_memory();
18531853
db.execute(
18541854
"CREATE TABLE uniq (id UUID PRIMARY KEY, email TEXT UNIQUE)",
@@ -1864,14 +1864,17 @@ fn con_02_unique_rejects_duplicate() {
18641864
)
18651865
.unwrap();
18661866

1867-
let result = db.execute(
1867+
db.execute(
18681868
"INSERT INTO uniq (id, email) VALUES ($id, $email)",
18691869
&params(vec![
18701870
("id", Value::Uuid(Uuid::new_v4())),
18711871
("email", Value::Text("alice@example.com".into())),
18721872
]),
1873-
);
1874-
assert!(result.is_err(), "UNIQUE column must reject duplicate value");
1873+
)
1874+
.unwrap();
1875+
1876+
let rows = db.scan("uniq", db.snapshot()).unwrap();
1877+
assert_eq!(rows.len(), 1, "duplicate UNIQUE insert must be a no-op");
18751878
}
18761879

18771880
#[test]
@@ -1901,7 +1904,7 @@ fn con_03_backward_compat_column_def_serde() {
19011904
}
19021905

19031906
#[test]
1904-
fn con_04_composite_unique_rejects_duplicate_tuple() {
1907+
fn con_04_composite_unique_duplicate_is_noop() {
19051908
let db = Database::open_memory();
19061909
db.execute(
19071910
"CREATE TABLE memberships (id UUID PRIMARY KEY, org_id UUID NOT NULL, email TEXT NOT NULL, UNIQUE (org_id, email))",
@@ -1919,18 +1922,18 @@ fn con_04_composite_unique_rejects_duplicate_tuple() {
19191922
)
19201923
.unwrap();
19211924

1922-
let result = db.execute(
1925+
db.execute(
19231926
"INSERT INTO memberships (id, org_id, email) VALUES ($id, $org_id, $email)",
19241927
&params(vec![
19251928
("id", Value::Uuid(Uuid::new_v4())),
19261929
("org_id", Value::Uuid(Uuid::from_u128(1))),
19271930
("email", Value::Text("alice@example.com".into())),
19281931
]),
1929-
);
1930-
assert!(
1931-
result.is_err(),
1932-
"duplicate composite tuple must be rejected"
1933-
);
1932+
)
1933+
.unwrap();
1934+
1935+
let rows = db.scan("memberships", db.snapshot()).unwrap();
1936+
assert_eq!(rows.len(), 1, "duplicate composite tuple must be a no-op");
19341937
}
19351938

19361939
#[test]
@@ -3617,16 +3620,16 @@ fn integrity_10_failed_insert_does_not_leak_memory() {
36173620
let baseline = accountant.usage().used;
36183621

36193622
for _ in 0..20 {
3620-
let result = db.execute(
3623+
db.execute(
36213624
"INSERT INTO items (id, name) VALUES ($id, 'dup')",
36223625
&params(vec![("id", Value::Uuid(Uuid::new_v4()))]),
3623-
);
3624-
assert!(result.is_err(), "duplicate UNIQUE insert must fail");
3626+
)
3627+
.unwrap();
36253628
}
36263629

36273630
let used = accountant.usage().used;
36283631
assert!(
36293632
used <= baseline + 256,
3630-
"failed inserts must not leak memory: baseline={baseline}, used={used}"
3633+
"duplicate no-op inserts must not leak memory: baseline={baseline}, used={used}"
36313634
);
36323635
}

0 commit comments

Comments
 (0)