From 20ecd3f57d4fb48a4d165be5f4d4a8391a08b95c Mon Sep 17 00:00:00 2001 From: Securifi Dev Date: Sun, 26 Jul 2026 06:07:37 +0000 Subject: [PATCH 1/4] fix(ddl): accept INT4/INT8/SMALLINT/INT2 in strict and kv schemas `document_strict` and `kv` engines validate declared column types through `ColumnType::from_str`, which only recognized `BIGINT`/`INT64`/`INTEGER`/`INT`. `CREATE COLLECTION` with an `INT4`, `INT8`, `SMALLINT`, or `INT2` column was rejected as an unknown type even though these are valid PostgreSQL integer aliases. All of them now map to the same `Int64` storage variant, since these engines always store integers as a full i64 regardless of declared width. Fixes #223. --- nodedb-types/src/columnar/column_parse.rs | 11 +- nodedb-types/src/columnar/column_type.rs | 14 ++ .../tests/ddl_int_width_aliases_strict_kv.rs | 130 ++++++++++++++++++ 3 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 nodedb/tests/ddl_int_width_aliases_strict_kv.rs diff --git a/nodedb-types/src/columnar/column_parse.rs b/nodedb-types/src/columnar/column_parse.rs index c8aae8a1f..5bb68e4f9 100644 --- a/nodedb-types/src/columnar/column_parse.rs +++ b/nodedb-types/src/columnar/column_parse.rs @@ -121,7 +121,16 @@ impl FromStr for ColumnType { } match upper.as_str() { - "BIGINT" | "INT64" | "INTEGER" | "INT" => Ok(Self::Int64), + // `INT4`/`INT8`/`SMALLINT`/`INT2` are PostgreSQL wire-width integer + // keywords (issue #223: strict/kv `CREATE COLLECTION` rejected + // them as unknown types even though they're valid aliases). They + // all collapse to the same `Int64` storage variant as + // `BIGINT`/`INTEGER`/`INT` — nodedb always stores integers as a + // full i64; the declared width only narrows the advertised wire + // OID (see `response_shape::schema::sql_data_type_to_ddl_col_type_with_raw`). + "BIGINT" | "INT64" | "INTEGER" | "INT" | "INT4" | "INT8" | "SMALLINT" | "INT2" => { + Ok(Self::Int64) + } "FLOAT64" | "DOUBLE" | "REAL" | "FLOAT" => Ok(Self::Float64), "TEXT" | "STRING" | "VARCHAR" => Ok(Self::String), "BOOL" | "BOOLEAN" => Ok(Self::Bool), diff --git a/nodedb-types/src/columnar/column_type.rs b/nodedb-types/src/columnar/column_type.rs index c8763eb82..570548047 100644 --- a/nodedb-types/src/columnar/column_type.rs +++ b/nodedb-types/src/columnar/column_type.rs @@ -286,6 +286,20 @@ mod tests { assert_eq!("UUID".parse::().unwrap(), ColumnType::Uuid); } + /// `INT4`/`INT8`/`SMALLINT`/`INT2` are wire-width DDL keywords (issue + /// #223: strict/kv `CREATE COLLECTION` rejected them as unknown column + /// types even though they're valid PostgreSQL integer aliases). They all + /// map to the same `Int64` storage variant as `BIGINT`/`INTEGER`/`INT` — + /// nodedb's columnar/strict/kv storage always keeps integers as a full + /// i64; only the wire (`DdlColType`) layer narrows the advertised OID. + #[test] + fn parse_int_width_aliases_map_to_int64() { + assert_eq!("INT4".parse::().unwrap(), ColumnType::Int64); + assert_eq!("INT8".parse::().unwrap(), ColumnType::Int64); + assert_eq!("SMALLINT".parse::().unwrap(), ColumnType::Int64); + assert_eq!("INT2".parse::().unwrap(), ColumnType::Int64); + } + #[test] fn parse_vector() { assert_eq!( diff --git a/nodedb/tests/ddl_int_width_aliases_strict_kv.rs b/nodedb/tests/ddl_int_width_aliases_strict_kv.rs new file mode 100644 index 000000000..4bb3c7cb6 --- /dev/null +++ b/nodedb/tests/ddl_int_width_aliases_strict_kv.rs @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Upstream issue #223 regression: `document_strict` and `kv` engines must +//! accept `INT4`/`SMALLINT` (and the other PostgreSQL integer-width +//! keywords) as valid column types. Both engines validate declared column +//! types via `nodedb_types::columnar::ColumnType::from_str` +//! (`nodedb-sql/src/ddl_ast/collection_type.rs`'s `build_strict_schema` / +//! `build_kv_collection_type`), which previously only recognized +//! `BIGINT`/`INT64`/`INTEGER`/`INT` — `INT4`, `INT8`, `SMALLINT`, and `INT2` +//! were rejected with "unknown column type", even though they're valid +//! PostgreSQL integer aliases. +//! +//! This is a DDL-acceptance smoke test only: for these engines the wire OID +//! stays `INT8` (20) regardless of declared width — `raw_type` refinement +//! (issue #217) is schemaless/columnar-only by design, since strict/kv +//! columns are typed authoritatively by the resolved `ColumnType`, not a +//! separately-tracked raw string. See `pgwire_ddl_result_types.rs`'s +//! `create_collection_int_widths_preserve_wire_oids` for the schemaless OID +//! fidelity coverage. + +mod common; + +use common::pgwire_harness::TestServer; + +/// `document_strict` `CREATE COLLECTION` with `INT4`/`SMALLINT` columns must +/// succeed (previously errored with "unknown column type: 'INT4'"). +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn strict_create_collection_accepts_int_width_aliases() { + let server = TestServer::start().await; + + server + .exec( + "CREATE COLLECTION strict_int_widths (\ + id TEXT PRIMARY KEY, \ + a INT4, \ + b SMALLINT, \ + c INT2, \ + d INT8\ + ) WITH (engine='document_strict')", + ) + .await + .expect("CREATE COLLECTION with INT4/SMALLINT/INT2/INT8 columns must succeed on document_strict"); + + server + .exec("INSERT INTO strict_int_widths (id, a, b, c, d) VALUES ('r1', 1, 2, 3, 4)") + .await + .expect("INSERT into strict_int_widths must succeed"); + + let stmt = server + .client + .prepare_typed( + "SELECT id, a, b, c, d FROM strict_int_widths WHERE id = $1", + &[tokio_postgres::types::Type::TEXT], + ) + .await + .expect("prepare strict_int_widths select"); + let rows = server + .client + .query(&stmt, &[&"r1"]) + .await + .expect("execute strict_int_widths select"); + assert_eq!(rows.len(), 1, "expected 1 row back from strict_int_widths"); + + // Load-bearing: document_strict stays INT8 (20) regardless of declared + // width — strict columns carry no `raw_type` (they're already typed + // authoritatively by `ColumnType`), so the issue #217 refinement never + // applies here. Width narrowing is schemaless/columnar-only by design. + for col_name in ["a", "b", "c", "d"] { + if let Some(col) = rows[0].columns().iter().find(|c| c.name() == col_name) { + assert_eq!( + col.type_().oid(), + 20, + "strict column '{col_name}' OID must stay INT8 (20), got {}", + col.type_().oid() + ); + } + } +} + +/// `kv` `CREATE COLLECTION` with `INT4`/`SMALLINT` columns must succeed +/// (previously errored with "unknown column type: 'INT4'"). The kv OID +/// stays INT8 (20) — do not assert a narrowed OID here. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kv_create_collection_accepts_int_width_aliases() { + let server = TestServer::start().await; + + server + .exec( + "CREATE COLLECTION kv_int_widths (\ + key TEXT PRIMARY KEY, \ + a INT4, \ + b SMALLINT\ + ) WITH (engine='kv')", + ) + .await + .expect("CREATE COLLECTION with INT4/SMALLINT columns must succeed on kv"); + + server + .exec("INSERT INTO kv_int_widths (key, a, b) VALUES ('r1', 1, 2)") + .await + .expect("INSERT into kv_int_widths must succeed"); + + let stmt = server + .client + .prepare_typed( + "SELECT key, a, b FROM kv_int_widths WHERE key = $1", + &[tokio_postgres::types::Type::TEXT], + ) + .await + .expect("prepare kv_int_widths select"); + let rows = server + .client + .query(&stmt, &[&"r1"]) + .await + .expect("execute kv_int_widths select"); + assert_eq!(rows.len(), 1, "expected 1 row back from kv_int_widths"); + + // Load-bearing: kv stays INT8 (20) — the fix does not widen/narrow kv's + // wire type, only schemaless/columnar's. + for col_name in ["a", "b"] { + if let Some(col) = rows[0].columns().iter().find(|c| c.name() == col_name) { + assert_eq!( + col.type_().oid(), + 20, + "kv column '{col_name}' OID must stay INT8 (20), got {}", + col.type_().oid() + ); + } + } +} From e0cb1835d74f483490835180532fa89aae729945 Mon Sep 17 00:00:00 2001 From: Securifi Dev Date: Sun, 26 Jul 2026 06:07:47 +0000 Subject: [PATCH 2/4] fix(pgwire): preserve declared integer width in RowDescription OIDs Schemaless/columnar collections resolve every integer width (SMALLINT, INT4, BIGINT, ...) to the single planner-facing `SqlDataType::Int64`, and `sql_data_type_to_ddl_col_type` mapped that straight to OID 20 (int8) for every column regardless of what was actually declared. Clients that declared `SMALLINT` or `INT4` got back OID 20 instead of the expected 21/23, which breaks ORMs and typed client libraries that trust the advertised wire OID. `SMALLINT`/`INT2` were also missing from the schemaless type-string parser and fell through to a `String` default, rendering as OID 25 (text) instead of an integer OID at all. The catalog now threads the column's raw declared type string through to the planner alongside its resolved `SqlDataType`, and a new `sql_data_type_to_ddl_col_type_with_raw` uses it to narrow an `Int8` result to `Int4`/`Int2` when the raw type says so. Only the wire-advertised width changes; storage stays a full i64 either way. Fixes #217. --- .../planner/catalog_adapter/type_convert.rs | 25 +++- .../planner/sql_plan_convert/output_schema.rs | 11 +- .../control/server/response_shape/schema.rs | 135 ++++++++++++++++++ nodedb/tests/pgwire_ddl_result_types.rs | 82 +++++++++++ nodedb/tests/pgwire_extended_query_engines.rs | 7 +- 5 files changed, 252 insertions(+), 8 deletions(-) diff --git a/nodedb/src/control/planner/catalog_adapter/type_convert.rs b/nodedb/src/control/planner/catalog_adapter/type_convert.rs index 65ba59aed..708428766 100644 --- a/nodedb/src/control/planner/catalog_adapter/type_convert.rs +++ b/nodedb/src/control/planner/catalog_adapter/type_convert.rs @@ -61,7 +61,12 @@ pub(super) fn convert_collection_type( nullable: true, is_primary_key: false, default: None, - raw_type: None, + // Mirrors the columnar branch below: the raw declared + // type string is the only place the wire-width refiner + // (`sql_data_type_to_ddl_col_type_with_raw`) can recover + // `SMALLINT`/`INT4` vs `BIGINT` — `parse_type_str` + // collapses all of them to the same `SqlDataType::Int64`. + raw_type: Some(type_str.clone()), }); } (EngineType::DocumentSchemaless, columns, Some(pk_name)) @@ -180,7 +185,7 @@ fn parse_type_str(s: &str) -> SqlDataType { return SqlDataType::Decimal; } match upper.as_str() { - "INT" | "INTEGER" | "INT4" | "INT8" | "BIGINT" => SqlDataType::Int64, + "INT" | "INTEGER" | "INT4" | "INT8" | "BIGINT" | "SMALLINT" | "INT2" => SqlDataType::Int64, "FLOAT" | "FLOAT4" | "FLOAT8" | "FLOAT64" | "DOUBLE" | "REAL" => SqlDataType::Float64, "BOOL" | "BOOLEAN" => SqlDataType::Bool, "BYTES" | "BYTEA" | "BLOB" => SqlDataType::Bytes, @@ -193,9 +198,23 @@ fn parse_type_str(s: &str) -> SqlDataType { mod tests { use nodedb_types::CollectionType; - use super::{SqlDataType, convert_collection_type}; + use super::{SqlDataType, convert_collection_type, parse_type_str}; use crate::control::security::catalog::StoredCollection; + /// `SMALLINT`/`INT2` are valid PostgreSQL wire-width integer keywords + /// (issue #217) that must resolve to the same `SqlDataType::Int64` arm as + /// `INT`/`INTEGER`/`INT4`/`INT8`/`BIGINT` — previously they were unlisted + /// and fell through to the `_ => SqlDataType::String` default, which is + /// what produced the wire OID 25 (text) bug for `SMALLINT` columns. + #[test] + fn parse_type_str_smallint_and_int2_map_to_int64() { + assert_eq!(parse_type_str("SMALLINT"), SqlDataType::Int64); + assert_eq!(parse_type_str("INT2"), SqlDataType::Int64); + // Case-insensitivity, matching every other arm in this function. + assert_eq!(parse_type_str("smallint"), SqlDataType::Int64); + assert_eq!(parse_type_str("int2"), SqlDataType::Int64); + } + /// A columnar (or spatial, which shares the same non-timeseries /// synthetic-PK path) collection whose DDL declares an explicit /// `id` field must not surface two `id` columns to the planner — diff --git a/nodedb/src/control/planner/sql_plan_convert/output_schema.rs b/nodedb/src/control/planner/sql_plan_convert/output_schema.rs index 710592079..e4d23f501 100644 --- a/nodedb/src/control/planner/sql_plan_convert/output_schema.rs +++ b/nodedb/src/control/planner/sql_plan_convert/output_schema.rs @@ -20,7 +20,7 @@ use nodedb_sql::types_expr::SqlExpr; use super::lateral::collection_name_from_plan; use super::output_schema_types::{infer_aggregate_type, infer_computed_expr_type}; use crate::control::server::response_shape::schema::{ - OutputColumn, OutputSchema, sql_data_type_to_ddl_col_type, + OutputColumn, OutputSchema, sql_data_type_to_ddl_col_type_with_raw, }; use crate::control::server::response_shape::types::DdlColType; @@ -94,7 +94,12 @@ fn column_types_for( Ok(Some(info)) => info .columns .iter() - .map(|c| (c.name.clone(), sql_data_type_to_ddl_col_type(&c.data_type))) + .map(|c| { + ( + c.name.clone(), + sql_data_type_to_ddl_col_type_with_raw(&c.data_type, c.raw_type.as_deref()), + ) + }) .collect(), _ => HashMap::new(), } @@ -173,7 +178,7 @@ fn ordered_columns_for( .map(|c| OutputColumn { display_name: c.name.clone(), lookup_key: c.name.clone(), - ty: sql_data_type_to_ddl_col_type(&c.data_type), + ty: sql_data_type_to_ddl_col_type_with_raw(&c.data_type, c.raw_type.as_deref()), }) .collect(), _ => Vec::new(), diff --git a/nodedb/src/control/server/response_shape/schema.rs b/nodedb/src/control/server/response_shape/schema.rs index 4467162c2..379d619da 100644 --- a/nodedb/src/control/server/response_shape/schema.rs +++ b/nodedb/src/control/server/response_shape/schema.rs @@ -63,12 +63,147 @@ pub fn sql_data_type_to_ddl_col_type( } } +/// Like [`sql_data_type_to_ddl_col_type`], but refines an `Int8` result using +/// the catalog's declared raw type string (`ColumnInfo::raw_type`), when +/// present. +/// +/// # Why only `Int8` is refined +/// +/// `SqlDataType::Int64` is the single planner-facing type for every integer +/// width — `SMALLINT`/`INT2`, `INTEGER`/`INT4`, and `BIGINT`/`INT8` all parse +/// to it (see `catalog_adapter::type_convert::parse_type_str` and +/// `nodedb_types::columnar::ColumnType::from_str`), because nodedb's +/// storage — columnar, strict, and kv alike — always keeps integers as a +/// full `i64`. The declared width therefore carries no storage or +/// computation meaning; it is authoritative for exactly one thing: the wire +/// type a pgwire client expects in `RowDescription`. A client that declared +/// `SMALLINT` expects OID 21 (and, for binary-format results, a 2-byte +/// value) — silently widening every integer to `BIGINT`'s OID 20 breaks ORMs +/// and typed client libraries that trust the advertised OID (nodedb upstream +/// issue #217). So this function narrows *display width only*, strictly +/// downstream of `sql_data_type_to_ddl_col_type`'s own type mapping: every +/// other `SqlDataType` variant (`String`, `Float64`, `Bool`, ...) passes +/// through unchanged, with `raw` ignored, because there is no other +/// wire-ambiguous case to resolve. +/// +/// # Matching +/// +/// `raw` is matched case-insensitively by keyword, using the same +/// prefix-with-boundary logic as +/// `pgwire::catalog::tables::collections::field_type_to_oid` (the +/// already-correct reference implementation this mirrors): `BIGINT`/`INT8` +/// are checked *first* so a plain `INT` prefix check can't accidentally +/// swallow `INT8` (there is no separator between `INT` and `8` for the +/// boundary check to reject). An unrecognized or absent `raw` leaves the +/// base `Int8` untouched — refinement is best-effort, never a hard error. +pub fn sql_data_type_to_ddl_col_type_with_raw( + ty: &nodedb_sql::types_expr::SqlDataType, + raw: Option<&str>, +) -> super::types::DdlColType { + use super::types::DdlColType; + + let base = sql_data_type_to_ddl_col_type(ty); + let (DdlColType::Int8, Some(raw)) = (base, raw) else { + return base; + }; + + let normalized = raw.trim().to_ascii_lowercase(); + let starts_with_type = |name: &str| { + normalized == name + || normalized.strip_prefix(name).is_some_and(|rest| { + rest.starts_with('(') || rest.chars().next().is_some_and(char::is_whitespace) + }) + }; + + if starts_with_type("bigint") || starts_with_type("int8") { + DdlColType::Int8 + } else if starts_with_type("smallint") || starts_with_type("int2") { + DdlColType::Int2 + } else if starts_with_type("integer") || starts_with_type("int4") || starts_with_type("int") { + DdlColType::Int4 + } else { + DdlColType::Int8 + } +} + #[cfg(test)] mod tests { use super::super::types::DdlColType; use super::*; use nodedb_sql::types_expr::SqlDataType; + /// `sql_data_type_to_ddl_col_type_with_raw` must narrow an `Int64` + /// (base `Int8`) column to `Int4`/`Int2` when the catalog's declared raw + /// type string says the DDL author wrote a narrower integer keyword. + /// `BIGINT`/`INT8` stay `Int8`. Case-insensitive, matching every other + /// raw-type match in this codebase (e.g. `field_type_to_oid`). + #[test] + fn refine_narrows_int8_by_raw_integer_keyword() { + let cases: &[(&str, DdlColType)] = &[ + ("BIGINT", DdlColType::Int8), + ("INT8", DdlColType::Int8), + ("INTEGER", DdlColType::Int4), + ("INT", DdlColType::Int4), + ("INT4", DdlColType::Int4), + ("SMALLINT", DdlColType::Int2), + ("INT2", DdlColType::Int2), + ]; + for (raw, expected) in cases { + assert_eq!( + sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::Int64, Some(raw)), + *expected, + "raw type {raw:?} must refine to {expected:?}" + ); + // Case-insensitivity. + assert_eq!( + sql_data_type_to_ddl_col_type_with_raw( + &SqlDataType::Int64, + Some(&raw.to_lowercase()) + ), + *expected, + "lowercase raw type {raw:?} must refine to {expected:?}" + ); + } + } + + /// A raw string that isn't a recognized integer keyword leaves the base + /// `Int8` untouched — the refinement is best-effort, never a hard error. + #[test] + fn refine_unrecognized_raw_keyword_stays_int8() { + assert_eq!( + sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::Int64, Some("NOT_A_TYPE")), + DdlColType::Int8 + ); + } + + /// `raw = None` (planner-synthesized columns, e.g. an auto-injected + /// `id`, carry no raw type) passes through to the unrefined base type. + #[test] + fn refine_with_no_raw_type_stays_base() { + assert_eq!( + sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::Int64, None), + DdlColType::Int8 + ); + } + + /// Only an `Int8` base is eligible for refinement — every other + /// `SqlDataType` passes through `sql_data_type_to_ddl_col_type` exactly, + /// with `raw` ignored even when it names an integer keyword (a raw type + /// string can never disagree with the planner's own resolved + /// `SqlDataType`, but this proves the fn doesn't misapply integer + /// narrowing to non-integer columns regardless). + #[test] + fn refine_passes_through_non_int8_types_untouched() { + assert_eq!( + sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::String, Some("SMALLINT")), + DdlColType::Text + ); + assert_eq!( + sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::Float64, None), + DdlColType::Float8 + ); + } + #[test] fn maps_every_sql_data_type_variant() { assert_eq!( diff --git a/nodedb/tests/pgwire_ddl_result_types.rs b/nodedb/tests/pgwire_ddl_result_types.rs index 932364b9e..280ef803e 100644 --- a/nodedb/tests/pgwire_ddl_result_types.rs +++ b/nodedb/tests/pgwire_ddl_result_types.rs @@ -27,6 +27,8 @@ use tokio::net::TcpStream; /// PostgreSQL built-in type OIDs (stable, wire-level constants). const OID_TEXT: u32 = 25; const OID_INT8: u32 = 20; +const OID_INT4: u32 = 23; +const OID_INT2: u32 = 21; /// A decoded simple-query result: the `RowDescription` columns as /// `(name, type_oid)` pairs, plus each `DataRow`'s fields as raw text @@ -262,3 +264,83 @@ async fn show_collections_row_description_preserves_int8_column() { "created_at INT8 value must round-trip as a decimal integer, got {created_at:?}" ); } + +/// Upstream issue #217 repro: integer OID wire fidelity. A schemaless +/// `CREATE COLLECTION` (no `WITH (engine=...)` clause) declaring every +/// PostgreSQL integer width — `INT`, `INTEGER`, `INT4`, `BIGINT`, `INT8`, +/// `SMALLINT`, `INT2` — must advertise each column's *own* wire OID in +/// `RowDescription`: 23 (int4) for the 4-byte aliases, 20 (int8) for the +/// 8-byte aliases, 21 (int2) for the 2-byte aliases. Before the fix, every +/// declared width collapsed to `SqlDataType::Int64`'s single wire mapping — +/// `INT`/`INTEGER`/`INT4`/`BIGINT`/`INT8` all rendered as OID 20 (int8), and +/// `SMALLINT`/`INT2` (unlisted in the type-string parser) fell through to +/// the `String` default and rendered as OID 25 (text) instead of an integer +/// OID at all. +#[tokio::test] +async fn create_collection_int_widths_preserve_wire_oids() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION fixprobe_ints (\ + id TEXT PRIMARY KEY, \ + a INT, \ + b INTEGER, \ + c INT4, \ + d BIGINT, \ + e INT8, \ + f SMALLINT, \ + g INT2\ + )", + ) + .await + .expect("CREATE COLLECTION fixprobe_ints must succeed"); + server + .exec( + "INSERT INTO fixprobe_ints (id, a, b, c, d, e, f, g) \ + VALUES ('r1', 1, 2, 3, 4, 5, 6, 7)", + ) + .await + .expect("INSERT fixprobe_ints must succeed"); + + let res = raw_simple_query( + server.pg_port, + "SELECT id, a, b, c, d, e, f, g FROM fixprobe_ints", + ) + .await; + + let expected = vec![ + ("id".to_string(), OID_TEXT), + ("a".to_string(), OID_INT4), + ("b".to_string(), OID_INT4), + ("c".to_string(), OID_INT4), + ("d".to_string(), OID_INT8), + ("e".to_string(), OID_INT8), + ("f".to_string(), OID_INT2), + ("g".to_string(), OID_INT2), + ]; + assert_eq!( + res.columns, expected, + "fixprobe_ints RowDescription (name, OID) must preserve each declared \ + integer width — INT/INTEGER/INT4 -> int4 (23), BIGINT/INT8 -> int8 (20), \ + SMALLINT/INT2 -> int2 (21)" + ); + + let row = res + .rows + .first() + .unwrap_or_else(|| panic!("expected 1 row, got: {:?}", res.rows)); + assert_eq!( + row, + &vec![ + Some("r1".to_string()), + Some("1".to_string()), + Some("2".to_string()), + Some("3".to_string()), + Some("4".to_string()), + Some("5".to_string()), + Some("6".to_string()), + Some("7".to_string()), + ], + "fixprobe_ints row values must round-trip unchanged" + ); +} diff --git a/nodedb/tests/pgwire_extended_query_engines.rs b/nodedb/tests/pgwire_extended_query_engines.rs index 0d205feb7..8ac52c94c 100644 --- a/nodedb/tests/pgwire_extended_query_engines.rs +++ b/nodedb/tests/pgwire_extended_query_engines.rs @@ -194,10 +194,13 @@ async fn extended_query_columnar_typed_scan() { let id: &str = rows[0].get("id"); assert_eq!(id, "r1"); - // RowDescription: id→TEXT(25), n→INT8(20), score→FLOAT8(701), flag→BOOL(16), label→TEXT(25). + // RowDescription: id→TEXT(25), n→INT4(23), score→FLOAT8(701), flag→BOOL(16), label→TEXT(25). + // `n` is declared `INT` in the DDL above, which is the INT4 alias — issue + // #217 fixed this column advertising OID 20 (int8) for every declared + // integer width; it now correctly narrows to OID 23 (int4). let expected: &[(&str, u32)] = &[ ("id", 25), - ("n", 20), + ("n", 23), ("score", 701), ("flag", 16), ("label", 25), From 90cf588a795ab7cebaf0f201b499c8477dd99970 Mon Sep 17 00:00:00 2001 From: Securifi Dev Date: Sun, 26 Jul 2026 06:17:16 +0000 Subject: [PATCH 3/4] test(pgwire): lock binary-format round-trip for narrowed integer OIDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extended-query typed reads (i16/i32/i64 via tokio_postgres) against a schemaless collection's SMALLINT/INT/BIGINT columns, asserting both the advertised OIDs (21/23/20) and the decoded values — locking the chain from declared width through RowDescription to the binary value encoders that the OID fix makes reachable. Also corrects an inaccurate sentence in the refinement fn's rustdoc (the keyword-match ordering is consistency with the reference implementation, not load-bearing). --- .../control/server/response_shape/schema.rs | 10 +-- nodedb/tests/pgwire_extended_query_engines.rs | 65 +++++++++++++++++++ 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/nodedb/src/control/server/response_shape/schema.rs b/nodedb/src/control/server/response_shape/schema.rs index 379d619da..71444302d 100644 --- a/nodedb/src/control/server/response_shape/schema.rs +++ b/nodedb/src/control/server/response_shape/schema.rs @@ -92,10 +92,12 @@ pub fn sql_data_type_to_ddl_col_type( /// prefix-with-boundary logic as /// `pgwire::catalog::tables::collections::field_type_to_oid` (the /// already-correct reference implementation this mirrors): `BIGINT`/`INT8` -/// are checked *first* so a plain `INT` prefix check can't accidentally -/// swallow `INT8` (there is no separator between `INT` and `8` for the -/// boundary check to reject). An unrecognized or absent `raw` leaves the -/// base `Int8` untouched — refinement is best-effort, never a hard error. +/// are checked *first* to mirror that reference implementation's ordering, +/// but the order isn't load-bearing — the boundary check itself already +/// rejects `int` as a prefix match of `int8`, since the next character +/// (`8`) is neither `(` nor whitespace. An unrecognized or absent `raw` +/// leaves the base `Int8` untouched — refinement is best-effort, never a +/// hard error. pub fn sql_data_type_to_ddl_col_type_with_raw( ty: &nodedb_sql::types_expr::SqlDataType, raw: Option<&str>, diff --git a/nodedb/tests/pgwire_extended_query_engines.rs b/nodedb/tests/pgwire_extended_query_engines.rs index 8ac52c94c..991f440b0 100644 --- a/nodedb/tests/pgwire_extended_query_engines.rs +++ b/nodedb/tests/pgwire_extended_query_engines.rs @@ -320,3 +320,68 @@ async fn extended_query_timeseries_null_param() { assert_eq!(rows.len(), 0, "NULL param must match 0 rows"); } + +// ── Schemaless engine (binary round-trip) ──────────────────────────────────── + +/// Binary-format round-trip for declared integer widths on a schemaless +/// collection. tokio_postgres decodes extended-query results per the +/// RowDescription OID, so reading each column back with a typed getter that +/// matches the *advertised* width (`i16`/`i32`/`i64`) fails the `get` if +/// either the OID or the binary payload width is wrong — this locks the +/// whole chain: declared width → OID 21/23/20 → binary encoder i16/i32/i64 +/// (issue #217). +#[tokio::test] +async fn extended_query_schemaless_int_width_binary_roundtrip() { + let srv = TestServer::start().await; + srv.exec("CREATE COLLECTION bin_widths (id TEXT PRIMARY KEY, s SMALLINT, i INT, b BIGINT)") + .await + .expect("CREATE COLLECTION bin_widths"); + srv.exec("INSERT INTO bin_widths (id, s, i, b) VALUES ('r1', 123, 456789, 9876543210)") + .await + .expect("INSERT bin_widths"); + + let stmt = srv + .client + .prepare_typed( + "SELECT id, s, i, b FROM bin_widths WHERE id = $1", + &[Type::TEXT], + ) + .await + .expect("prepare bin_widths select"); + + let rows = srv + .client + .query(&stmt, &[&"r1"]) + .await + .expect("execute bin_widths select"); + + assert_eq!(rows.len(), 1, "expected 1 row"); + + // Typed getters matching the advertised OIDs — a wrong OID or a + // wrong-width binary payload panics inside `get` before we ever reach + // the assertions below. + let s = rows[0].get::<_, i16>("s"); + let i = rows[0].get::<_, i32>("i"); + let b = rows[0].get::<_, i64>("b"); + assert_eq!(s, 123); + assert_eq!(i, 456789); + assert_eq!(b, 9876543210); + + // RowDescription OID check: s→INT2(21), i→INT4(23), b→INT8(20). + let expected: &[(&str, u32)] = &[("s", 21), ("i", 23), ("b", 20)]; + for (col_name, expected_oid) in expected { + let col = rows[0] + .columns() + .iter() + .find(|c| c.name() == *col_name) + .unwrap_or_else(|| panic!("column '{col_name}' must appear in RowDescription")); + assert_eq!( + col.type_().oid(), + *expected_oid, + "bin_widths column '{}' OID must be {}, got {}", + col_name, + expected_oid, + col.type_().oid() + ); + } +} From 1d2c82bf571e457eca7869e77ada02e5bbb209a9 Mon Sep 17 00:00:00 2001 From: Farhan Syah Date: Sun, 26 Jul 2026 22:38:09 +0800 Subject: [PATCH 4/4] fix(sql): enforce declared integer widths on write and keep them in sync across DDL Narrowing a column's RowDescription OID to its declared width (issue #217) is only honest if the stored value actually fits: nodedb keeps every integer as a full i64, so nothing previously stopped a value wider than SMALLINT/INTEGER from being written into a column that advertises a 2- or 4-byte wire type, corrupting the value in transit. Introduce IntWidth as the single source of truth mapping a declared type string to its range, PostgreSQL OID, and type name, and route every consumer through it: write-time range validation in the planner (INSERT/UPSERT/UPDATE, including KV's separate insert path), a defence-in-depth check on the read path for data that predates the constraint, catalog introspection OIDs, and RowDescription narrowing (replacing the old raw-string refinement). ALTER ADD/RENAME/DROP COLUMN and ALTER COLUMN TYPE now keep the catalog's declared-type record in sync so width fidelity survives schema changes instead of silently reverting to BIGINT. Also fold the sign into negative integer literals during AST conversion so i64::MIN parses exactly instead of falling back to an approximate float. --- nodedb-sql/src/error.rs | 15 + nodedb-sql/src/planner/dml.rs | 11 +- nodedb-sql/src/planner/dml_helpers.rs | 84 +++++ nodedb-sql/src/planner/dml_update_delete.rs | 6 +- nodedb-sql/src/resolver/columns.rs | 3 + nodedb-sql/src/resolver/expr/convert.rs | 32 +- nodedb-sql/src/types/collection.rs | 10 + .../tests/positional_insert_column_binding.rs | 2 + nodedb-types/src/columnar/column_parse.rs | 5 +- nodedb-types/src/columnar/int_width.rs | 233 ++++++++++++ nodedb-types/src/columnar/mod.rs | 2 + .../planner/catalog_adapter/type_convert.rs | 67 +++- .../planner/sql_plan_convert/output_schema.rs | 7 +- .../control/server/pgwire/catalog/schema.rs | 1 + .../pgwire/catalog/tables/collections.rs | 18 +- .../server/pgwire/handler/shape_encode.rs | 64 +++- .../control/server/response_shape/schema.rs | 157 ++++---- .../neutral/collection/alter/add_column.rs | 19 + .../neutral/collection/alter/alter_type.rs | 7 +- .../neutral/collection/alter/drop_column.rs | 5 +- .../neutral/collection/alter/rename_column.rs | 5 +- .../neutral/collection/alter/strict_schema.rs | 42 +++ .../tests/ddl_int_width_aliases_strict_kv.rs | 176 ++++++--- nodedb/tests/pgwire_extended_query.rs | 11 +- nodedb/tests/pgwire_extended_query_engines.rs | 8 +- .../pgwire_int_width_range_enforcement.rs | 345 ++++++++++++++++++ 26 files changed, 1161 insertions(+), 174 deletions(-) create mode 100644 nodedb-types/src/columnar/int_width.rs create mode 100644 nodedb/tests/pgwire_int_width_range_enforcement.rs diff --git a/nodedb-sql/src/error.rs b/nodedb-sql/src/error.rs index efe39888a..5447e3415 100644 --- a/nodedb-sql/src/error.rs +++ b/nodedb-sql/src/error.rs @@ -20,6 +20,21 @@ pub enum SqlError { #[error("type mismatch: {detail}")] TypeMismatch { detail: String }, + /// A write supplied an integer wider than the column's declared type. + /// + /// nodedb stores every integer as an `i64`, so this is not a storage + /// limit — it is the constraint that makes the column's advertised wire + /// type honest. A column declared `INTEGER` reports OID 23, and a pgwire + /// client reading it in binary format decodes exactly four bytes; letting + /// a wider value in would mean either truncating it on read or lying about + /// the type. PostgreSQL rejects the same write with the same message. + #[error("value {value} is out of range for column '{column}' of type {declared_type}")] + IntegerOutOfRange { + column: String, + value: i64, + declared_type: &'static str, + }, + #[error("unsupported: {detail}")] Unsupported { detail: String }, diff --git a/nodedb-sql/src/planner/dml.rs b/nodedb-sql/src/planner/dml.rs index bd094006c..755b14634 100644 --- a/nodedb-sql/src/planner/dml.rs +++ b/nodedb-sql/src/planner/dml.rs @@ -6,8 +6,8 @@ use nodedb_types::DatabaseId; use sqlparser::ast::{self}; use super::dml_helpers::{ - build_kv_insert_plan, build_vector_primary_insert_plan, convert_value_rows, - resolve_insert_columns, + build_kv_insert_plan, build_vector_primary_insert_plan, check_declared_int_ranges, + convert_value_rows, resolve_insert_columns, }; use crate::engine_rules::{self, InsertParams}; use crate::error::{Result, SqlError}; @@ -135,6 +135,7 @@ pub fn plan_insert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result Result = info .columns .iter() @@ -218,6 +221,7 @@ pub fn plan_upsert(ins: &ast::Insert, catalog: &dyn SqlCatalog) -> Result Result = info .columns .iter() @@ -294,6 +299,7 @@ fn plan_upsert_with_on_conflict( KvInsertIntent::Put, on_conflict_updates, info.primary_key.as_deref(), + &info.columns, ); } @@ -302,6 +308,7 @@ fn plan_upsert_with_on_conflict( let columns = resolve_insert_columns(columns, &info, rows_ast)?; let rows = convert_value_rows(&columns, rows_ast)?; + check_declared_int_ranges(&info.columns, &rows)?; let column_defaults: Vec<(String, String)> = info .columns .iter() diff --git a/nodedb-sql/src/planner/dml_helpers.rs b/nodedb-sql/src/planner/dml_helpers.rs index a820e6c75..f5644baad 100644 --- a/nodedb-sql/src/planner/dml_helpers.rs +++ b/nodedb-sql/src/planner/dml_helpers.rs @@ -25,6 +25,85 @@ pub(super) fn convert_value_rows( .collect() } +/// Reject any integer value that does not fit its column's declared width. +/// +/// nodedb stores every integer as an `i64`, so this is not a storage limit. +/// It is the constraint that makes the column's advertised wire type honest: +/// a column declared `INTEGER` reports OID 23 in `RowDescription`, and a +/// pgwire client reading it in binary format decodes exactly four bytes. +/// Accepting a wider value would force a later choice between truncating it on +/// read and lying about the column's type — so the value is refused at the +/// point it enters, exactly as PostgreSQL refuses it. +/// +/// This runs in the planner rather than in each engine because the declared +/// width is engine-independent (the same `IntWidth` drives the wire type for +/// schemaless, columnar, strict, and kv alike), and because parameters are +/// bound into the AST before planning — so one check here covers both literal +/// `VALUES` and `$1` placeholders, for every engine, on every DML path. +/// +/// Non-integer values and columns with no declared width pass through: this +/// checks range only, never type. +pub(super) fn check_declared_int_ranges( + columns: &[ColumnInfo], + rows: &[Vec<(String, SqlValue)>], +) -> Result<()> { + // Overwhelmingly the common case — skip the per-cell name lookup entirely + // when the collection declares no narrowed integer column. + if !columns.iter().any(|c| { + matches!( + c.int_width, + Some(nodedb_types::columnar::IntWidth::I16 | nodedb_types::columnar::IntWidth::I32) + ) + }) { + return Ok(()); + } + + for row in rows { + for (name, value) in row { + let SqlValue::Int(v) = value else { continue }; + let Some(width) = columns + .iter() + .find(|c| c.name.eq_ignore_ascii_case(name)) + .and_then(|c| c.int_width) + else { + continue; + }; + if !width.contains(*v) { + return Err(SqlError::IntegerOutOfRange { + column: name.clone(), + value: *v, + declared_type: width.pg_type_name(), + }); + } + } + } + Ok(()) +} + +/// [`check_declared_int_ranges`] for `UPDATE ... SET col = `. +/// +/// Only literal assignments are checkable at plan time; a computed assignment +/// (`SET n = n + 1`) has no value until the Data Plane evaluates it. Those are +/// caught on the read path instead, where the encoder refuses to transmit a +/// value that does not fit the column's advertised width — so an out-of-range +/// value can never reach a client silently by either route. +pub(super) fn check_declared_int_ranges_in_assignments( + columns: &[ColumnInfo], + assignments: &[(String, SqlExpr)], +) -> Result<()> { + let literals: Vec<(String, SqlValue)> = assignments + .iter() + .filter_map(|(col, expr)| match expr { + SqlExpr::Literal(v @ SqlValue::Int(_)) => Some((col.clone(), v.clone())), + _ => None, + }) + .collect(); + if literals.is_empty() { + return Ok(()); + } + check_declared_int_ranges(columns, std::slice::from_ref(&literals)) +} + /// Resolve the effective column list for a `VALUES`-clause INSERT/UPSERT. /// /// A *positional* insert — `INSERT INTO t VALUES (...)` with no explicit @@ -277,6 +356,7 @@ pub(super) fn build_kv_insert_plan( intent: KvInsertIntent, on_conflict_updates: Vec<(String, SqlExpr)>, pk_col: Option<&str>, + declared_columns: &[ColumnInfo], ) -> Result> { // Positional KV insert (no column list): the key/value split below is // driven entirely by matching column *names* against `key_col_name`/ @@ -288,6 +368,10 @@ pub(super) fn build_kv_insert_plan( collection: table_name, }); } + // KV returns early from every INSERT/UPSERT entry point, so the declared + // width check lives here rather than at the call sites — otherwise a + // fourth KV entry point could be added without it. + check_declared_int_ranges(declared_columns, &convert_value_rows(columns, rows_ast)?)?; let key_col_name = pk_col.unwrap_or("key"); let key_idx = columns.iter().position(|c| c == key_col_name); let ttl_idx = columns.iter().position(|c| c == "ttl"); diff --git a/nodedb-sql/src/planner/dml_update_delete.rs b/nodedb-sql/src/planner/dml_update_delete.rs index bef2e25c2..7de1d4e11 100644 --- a/nodedb-sql/src/planner/dml_update_delete.rs +++ b/nodedb-sql/src/planner/dml_update_delete.rs @@ -8,7 +8,10 @@ use sqlparser::ast; use super::super::ast_helpers::{ flatten_and_expr, qualified_ident_pair, strip_and_convert_filters, }; -use super::super::dml_helpers::{extract_point_keys, extract_table_name_from_table_with_joins}; +use super::super::dml_helpers::{ + check_declared_int_ranges_in_assignments, extract_point_keys, + extract_table_name_from_table_with_joins, +}; use crate::engine_rules::{self, DeleteParams, UpdateFromParams, UpdateParams}; use crate::error::{Result, SqlError}; use crate::parser::normalize::{ @@ -41,6 +44,7 @@ pub fn plan_update(stmt: &ast::Statement, catalog: &dyn SqlCatalog) -> Result super::super::select::convert_where_to_filters(expr)?, diff --git a/nodedb-sql/src/resolver/columns.rs b/nodedb-sql/src/resolver/columns.rs index d975f84e2..9e7823a74 100644 --- a/nodedb-sql/src/resolver/columns.rs +++ b/nodedb-sql/src/resolver/columns.rs @@ -285,6 +285,7 @@ fn array_columns(view: &ArrayCatalogView) -> Vec { is_primary_key: false, default: None, raw_type: None, + int_width: None, }); } for a in &view.attrs { @@ -295,6 +296,7 @@ fn array_columns(view: &ArrayCatalogView) -> Vec { is_primary_key: false, default: None, raw_type: None, + int_width: None, }); } cols @@ -356,6 +358,7 @@ mod tests { is_primary_key: false, default: None, raw_type: None, + int_width: None, }) .collect(), primary_key: None, diff --git a/nodedb-sql/src/resolver/expr/convert.rs b/nodedb-sql/src/resolver/expr/convert.rs index da5fc1112..1b5d647dc 100644 --- a/nodedb-sql/src/resolver/expr/convert.rs +++ b/nodedb-sql/src/resolver/expr/convert.rs @@ -2,7 +2,7 @@ //! Convert sqlparser AST expressions to our SqlExpr IR. -use sqlparser::ast::{self, Expr, Value}; +use sqlparser::ast::{self, Expr, UnaryOperator, Value}; use crate::error::{Result, SqlError}; use crate::parser::normalize::{SCHEMA_QUALIFIED_MSG, normalize_ident}; @@ -130,6 +130,36 @@ fn convert_expr_inner(expr: &Expr, depth: &mut usize) -> Result { right: Box::new(convert_expr_depth(right, depth)?), }) } + // A negative integer literal reaches sqlparser as unary minus applied + // to a *positive* number, so the most negative `BIGINT` arrives as + // `-(9223372036854775808)` — and that operand does not fit an `i64`. + // Converting the operand on its own therefore falls back to `Float` + // and silently turns an exact integer into an approximate one. Folding + // the sign into the literal before parsing keeps the whole `i64` range + // exact; anything that still does not fit falls through to the general + // path below and is handled as before. + Expr::UnaryOp { + op: UnaryOperator::Minus, + expr: inner, + } if matches!( + inner.as_ref(), + Expr::Value(v) if matches!(&v.value, Value::Number(..)) + ) => + { + let Expr::Value(v) = inner.as_ref() else { + unreachable!("guarded by the `matches!` above") + }; + let Value::Number(n, _) = &v.value else { + unreachable!("guarded by the `matches!` above") + }; + match format!("-{n}").parse::() { + Ok(i) => Ok(SqlExpr::Literal(SqlValue::Int(i))), + Err(_) => Ok(SqlExpr::UnaryOp { + op: UnaryOp::Neg, + expr: Box::new(convert_expr_depth(inner, depth)?), + }), + } + } Expr::UnaryOp { op, expr } => Ok(SqlExpr::UnaryOp { op: convert_unary_op(op)?, expr: Box::new(convert_expr_depth(expr, depth)?), diff --git a/nodedb-sql/src/types/collection.rs b/nodedb-sql/src/types/collection.rs index 6bd5eff76..d0be206b2 100644 --- a/nodedb-sql/src/types/collection.rs +++ b/nodedb-sql/src/types/collection.rs @@ -76,4 +76,14 @@ pub struct ColumnInfo { /// Columnar INSERT converters use this to reconstruct the exact `ColumnType` /// so JSON / Geometry / UUID columns are not incorrectly inferred as String. pub raw_type: Option, + /// Declared width of an integer column, resolved once from the catalog at + /// adapter-construction time. `None` for non-integer columns and for + /// integer columns whose declared type carried no width. + /// + /// This is deliberately a resolved [`IntWidth`] rather than another raw + /// string: it is read on both the write path (range validation) and the + /// read path (`RowDescription` OID and binary payload width), and those + /// two must agree exactly. Resolving once at the catalog boundary is what + /// makes disagreement unrepresentable. + pub int_width: Option, } diff --git a/nodedb-sql/tests/positional_insert_column_binding.rs b/nodedb-sql/tests/positional_insert_column_binding.rs index f49f5eced..be9fd6f15 100644 --- a/nodedb-sql/tests/positional_insert_column_binding.rs +++ b/nodedb-sql/tests/positional_insert_column_binding.rs @@ -40,6 +40,7 @@ impl SqlCatalog for Catalog { is_primary_key: true, default: None, raw_type: Some("INT".into()), + int_width: Some(nodedb_types::columnar::IntWidth::I32), }, ColumnInfo { name: "note".into(), @@ -48,6 +49,7 @@ impl SqlCatalog for Catalog { is_primary_key: false, default: None, raw_type: Some("TEXT".into()), + int_width: None, }, ], primary_key: Some("id".into()), diff --git a/nodedb-types/src/columnar/column_parse.rs b/nodedb-types/src/columnar/column_parse.rs index 5bb68e4f9..6faaae0e4 100644 --- a/nodedb-types/src/columnar/column_parse.rs +++ b/nodedb-types/src/columnar/column_parse.rs @@ -126,8 +126,9 @@ impl FromStr for ColumnType { // them as unknown types even though they're valid aliases). They // all collapse to the same `Int64` storage variant as // `BIGINT`/`INTEGER`/`INT` — nodedb always stores integers as a - // full i64; the declared width only narrows the advertised wire - // OID (see `response_shape::schema::sql_data_type_to_ddl_col_type_with_raw`). + // full i64. The declared width is carried separately as an + // [`super::IntWidth`], which is what bounds writes and narrows the + // advertised wire OID; it is deliberately not a storage variant. "BIGINT" | "INT64" | "INTEGER" | "INT" | "INT4" | "INT8" | "SMALLINT" | "INT2" => { Ok(Self::Int64) } diff --git a/nodedb-types/src/columnar/int_width.rs b/nodedb-types/src/columnar/int_width.rs new file mode 100644 index 000000000..8e774f807 --- /dev/null +++ b/nodedb-types/src/columnar/int_width.rs @@ -0,0 +1,233 @@ +// SPDX-License-Identifier: Apache-2.0 + +//! [`IntWidth`] — the declared width of an integer column. +//! +//! nodedb stores every integer as a full `i64` regardless of the width the +//! DDL author declared: `ColumnType::Int64` is the one storage variant for +//! `SMALLINT`, `INTEGER`, and `BIGINT` alike. The declared width is therefore +//! not a storage property — it is a *constraint plus a wire contract*: +//! +//! * **Constraint** — a value written to a column declared `INTEGER` must fit +//! in an `i32`, exactly as PostgreSQL enforces (`integer out of range`). +//! Without this, the wire contract below cannot be honoured. +//! * **Wire contract** — the column's `RowDescription` OID is 21/23/20, and +//! under the binary result format the value is transmitted as 2/4/8 bytes. +//! A client that declared `SMALLINT` decodes exactly two bytes; handing it +//! a wider value is silent corruption, not a rounding error. +//! +//! Because both of those depend on recognising the same set of SQL keywords, +//! [`IntWidth::from_declared_type`] is the **single** place that maps a +//! declared type string to a width. Every consumer — catalog introspection +//! OIDs, `RowDescription` OIDs, and write-time range validation — resolves +//! through it, so the three can never drift apart. + +use serde::{Deserialize, Serialize}; + +/// The declared width of an integer column. +/// +/// Ordered narrow → wide; `I64` is the storage width and the default for an +/// integer column whose declared type carries no width information. +#[derive( + Debug, + Clone, + Copy, + PartialEq, + Eq, + PartialOrd, + Ord, + Hash, + Serialize, + Deserialize, + zerompk::ToMessagePack, + zerompk::FromMessagePack, +)] +#[msgpack(c_enum)] +#[repr(u8)] +pub enum IntWidth { + /// `SMALLINT` / `INT2` — 2 bytes on the wire, PostgreSQL OID 21. + I16 = 0, + /// `INTEGER` / `INT` / `INT4` — 4 bytes on the wire, PostgreSQL OID 23. + I32 = 1, + /// `BIGINT` / `INT8` / `INT64` — 8 bytes on the wire, PostgreSQL OID 20. + I64 = 2, +} + +impl IntWidth { + /// Resolve a declared SQL type string to an integer width, or `None` when + /// the string does not name an integer type. + /// + /// Matching is case-insensitive and boundary-aware: a keyword matches when + /// the (trimmed, lowercased) input either equals it exactly or is followed + /// by `(` or whitespace, so trailing modifiers (`"BIGINT NOT NULL"`) and + /// parameterised spellings still resolve. The boundary check is also what + /// keeps `int` from swallowing `int2`/`int4`/`int8`/`int64` as prefixes — + /// the next character is a digit, which is neither `(` nor whitespace — + /// so the arm order below is not load-bearing. + /// + /// This is the single source of truth for keyword → width. Callers that + /// need an OID or a range check derive it from the returned width rather + /// than re-parsing the string. + pub fn from_declared_type(declared: &str) -> Option { + let normalized = declared.trim().to_ascii_lowercase(); + let is = |name: &str| { + normalized == name + || normalized.strip_prefix(name).is_some_and(|rest| { + rest.starts_with('(') || rest.chars().next().is_some_and(char::is_whitespace) + }) + }; + + if is("smallint") || is("int2") { + Some(Self::I16) + } else if is("integer") || is("int4") || is("int") { + Some(Self::I32) + } else if is("bigint") || is("int8") || is("int64") { + Some(Self::I64) + } else { + None + } + } + + /// The inclusive value range a column of this width accepts. + pub const fn range(self) -> (i64, i64) { + match self { + Self::I16 => (i16::MIN as i64, i16::MAX as i64), + Self::I32 => (i32::MIN as i64, i32::MAX as i64), + Self::I64 => (i64::MIN, i64::MAX), + } + } + + /// Whether `value` fits in a column of this declared width. + pub const fn contains(self, value: i64) -> bool { + let (min, max) = self.range(); + value >= min && value <= max + } + + /// The PostgreSQL type OID a column of this width advertises in + /// `RowDescription` and in catalog introspection. + pub const fn pg_oid(self) -> u32 { + match self { + Self::I16 => 21, + Self::I32 => 23, + Self::I64 => 20, + } + } + + /// The canonical PostgreSQL type name, for error messages that must read + /// like PostgreSQL's own (`integer out of range`). + pub const fn pg_type_name(self) -> &'static str { + match self { + Self::I16 => "smallint", + Self::I32 => "integer", + Self::I64 => "bigint", + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn resolves_every_declared_integer_spelling() { + let cases: &[(&str, IntWidth)] = &[ + ("SMALLINT", IntWidth::I16), + ("INT2", IntWidth::I16), + ("INTEGER", IntWidth::I32), + ("INT", IntWidth::I32), + ("INT4", IntWidth::I32), + ("BIGINT", IntWidth::I64), + ("INT8", IntWidth::I64), + ("INT64", IntWidth::I64), + ]; + for (declared, expected) in cases { + assert_eq!( + IntWidth::from_declared_type(declared), + Some(*expected), + "{declared} must resolve to {expected:?}" + ); + assert_eq!( + IntWidth::from_declared_type(&declared.to_lowercase()), + Some(*expected), + "{declared} must resolve case-insensitively" + ); + } + } + + /// The boundary check — not the arm order — is what stops `int` from + /// prefix-matching the numbered spellings. Locking it directly means a + /// future reordering of the arms cannot silently regress the mapping. + #[test] + fn int_does_not_prefix_match_numbered_spellings() { + for (declared, expected) in [ + ("int2", IntWidth::I16), + ("int4", IntWidth::I32), + ("int8", IntWidth::I64), + ("int64", IntWidth::I64), + ] { + assert_eq!(IntWidth::from_declared_type(declared), Some(expected)); + } + } + + #[test] + fn tolerates_trailing_modifiers_and_parameters() { + assert_eq!( + IntWidth::from_declared_type("BIGINT NOT NULL"), + Some(IntWidth::I64) + ); + assert_eq!( + IntWidth::from_declared_type(" smallint "), + Some(IntWidth::I16) + ); + assert_eq!(IntWidth::from_declared_type("int(11)"), Some(IntWidth::I32)); + } + + #[test] + fn rejects_non_integer_types() { + for declared in [ + "TEXT", + "VARCHAR(20)", + "FLOAT8", + "BOOL", + "TIMESTAMP", + "GEOMETRY", + "", + "intergalactic", + ] { + assert_eq!( + IntWidth::from_declared_type(declared), + None, + "{declared} must not resolve to an integer width" + ); + } + } + + #[test] + fn range_matches_the_rust_primitive_it_names() { + assert_eq!(IntWidth::I16.range(), (i16::MIN as i64, i16::MAX as i64)); + assert_eq!(IntWidth::I32.range(), (i32::MIN as i64, i32::MAX as i64)); + assert_eq!(IntWidth::I64.range(), (i64::MIN, i64::MAX)); + } + + #[test] + fn contains_rejects_values_one_past_the_boundary() { + assert!(IntWidth::I16.contains(i16::MAX as i64)); + assert!(!IntWidth::I16.contains(i16::MAX as i64 + 1)); + assert!(IntWidth::I16.contains(i16::MIN as i64)); + assert!(!IntWidth::I16.contains(i16::MIN as i64 - 1)); + + assert!(IntWidth::I32.contains(i32::MAX as i64)); + assert!(!IntWidth::I32.contains(i32::MAX as i64 + 1)); + assert!(IntWidth::I32.contains(i32::MIN as i64)); + assert!(!IntWidth::I32.contains(i32::MIN as i64 - 1)); + + assert!(IntWidth::I64.contains(i64::MAX)); + assert!(IntWidth::I64.contains(i64::MIN)); + } + + #[test] + fn pg_oids_match_the_postgres_catalog() { + assert_eq!(IntWidth::I16.pg_oid(), 21); + assert_eq!(IntWidth::I32.pg_oid(), 23); + assert_eq!(IntWidth::I64.pg_oid(), 20); + } +} diff --git a/nodedb-types/src/columnar/mod.rs b/nodedb-types/src/columnar/mod.rs index 4bcbb8cf9..a661b6a5f 100644 --- a/nodedb-types/src/columnar/mod.rs +++ b/nodedb-types/src/columnar/mod.rs @@ -4,6 +4,7 @@ pub mod column_def; pub mod column_parse; pub mod column_type; pub mod dml_wal_record; +pub mod int_width; pub mod profile; pub mod schema; pub mod wal_record; @@ -12,6 +13,7 @@ pub use column_def::{ColumnDef, ColumnModifier}; pub use column_parse::ColumnTypeParseError; pub use column_type::ColumnType; pub use dml_wal_record::ColumnarDmlWalRecord; +pub use int_width::IntWidth; pub use profile::{ColumnarProfile, DocumentMode}; pub use schema::{ BITEMPORAL_RESERVED_COLUMNS, BITEMPORAL_SYSTEM_FROM, BITEMPORAL_VALID_FROM, diff --git a/nodedb/src/control/planner/catalog_adapter/type_convert.rs b/nodedb/src/control/planner/catalog_adapter/type_convert.rs index 708428766..cbdbc6c6d 100644 --- a/nodedb/src/control/planner/catalog_adapter/type_convert.rs +++ b/nodedb/src/control/planner/catalog_adapter/type_convert.rs @@ -3,6 +3,7 @@ //! Conversion helpers: `StoredCollection` → planner-facing catalog types. use nodedb_sql::types::{ColumnInfo, EngineType, SqlDataType}; +use nodedb_types::columnar::IntWidth; /// Convert a StoredCollection to engine type, columns, and primary key. pub(super) fn convert_collection_type( @@ -11,6 +12,20 @@ pub(super) fn convert_collection_type( use nodedb_types::CollectionType; use nodedb_types::columnar::DocumentMode; + // Declared integer widths, resolved once per collection from the raw DDL + // type strings the catalog records in `fields` for *every* engine. + // + // Strict and KV columns are typed by a resolved `ColumnType`, which + // deliberately has one `Int64` variant for every declared width (nodedb + // stores all integers as i64). `fields` is therefore the only surviving + // record of what the author actually wrote, and it is populated for + // strict/KV exactly as it is for schemaless/columnar — see + // `ddl::neutral::collection::create::build`, which fills it from the raw + // column list before the typed schema is built. Resolving from it here + // keeps declared-width fidelity uniform across all engines without + // widening any persisted structure. + let declared_widths = declared_int_widths(&stored.fields); + match &stored.collection_type { CollectionType::Document(DocumentMode::Strict(schema)) => { let columns = schema @@ -23,6 +38,7 @@ pub(super) fn convert_collection_type( is_primary_key: c.primary_key, default: c.default.clone(), raw_type: None, + int_width: lookup_int_width(&declared_widths, &c.name), }) .collect(); let pk = schema @@ -49,6 +65,7 @@ pub(super) fn convert_collection_type( is_primary_key: true, default: None, raw_type: None, + int_width: None, }]; // Add tracked fields from catalog. for (name, type_str) in &stored.fields { @@ -61,12 +78,8 @@ pub(super) fn convert_collection_type( nullable: true, is_primary_key: false, default: None, - // Mirrors the columnar branch below: the raw declared - // type string is the only place the wire-width refiner - // (`sql_data_type_to_ddl_col_type_with_raw`) can recover - // `SMALLINT`/`INT4` vs `BIGINT` — `parse_type_str` - // collapses all of them to the same `SqlDataType::Int64`. - raw_type: Some(type_str.clone()), + raw_type: None, + int_width: IntWidth::from_declared_type(type_str), }); } (EngineType::DocumentSchemaless, columns, Some(pk_name)) @@ -84,6 +97,7 @@ pub(super) fn convert_collection_type( is_primary_key: c.primary_key, default: c.default.clone(), raw_type: None, + int_width: lookup_int_width(&declared_widths, &c.name), }) .collect(); let pk = config @@ -121,6 +135,7 @@ pub(super) fn convert_collection_type( Some((_, type_str)) => (parse_type_str(type_str), None, Some(type_str.clone())), None => (SqlDataType::String, Some("UUID_V7".into()), None), }; + let pk_width = pk_raw.as_deref().and_then(IntWidth::from_declared_type); columns.push(ColumnInfo { name: pk_name.into(), data_type: pk_type, @@ -128,6 +143,7 @@ pub(super) fn convert_collection_type( is_primary_key: true, default: pk_default, raw_type: pk_raw, + int_width: pk_width, }); } for (name, type_str) in &stored.fields { @@ -141,6 +157,7 @@ pub(super) fn convert_collection_type( is_primary_key: false, default: None, raw_type: Some(type_str.clone()), + int_width: IntWidth::from_declared_type(type_str), }); } let pk = if profile.is_timeseries() { @@ -153,6 +170,35 @@ pub(super) fn convert_collection_type( } } +/// Resolve the declared integer width of every catalog field that names an +/// integer type, keyed by column name. +/// +/// Non-integer fields are dropped rather than stored as `None`, so the result +/// is usually empty and the common case costs one allocation of zero capacity. +fn declared_int_widths(fields: &[(String, String)]) -> Vec<(&str, IntWidth)> { + fields + .iter() + .filter_map(|(name, type_str)| { + IntWidth::from_declared_type(type_str).map(|w| (name.as_str(), w)) + }) + .collect() +} + +/// Look up a column's declared integer width by name, case-insensitively to +/// match the rest of this module's column-name comparisons. +/// +/// `None` means either "not an integer column" or "the catalog has no record +/// of this column's declared type" — for example a column added by +/// `ALTER ADD COLUMN`, whose declared width was never recorded in `fields`. +/// Both degrade to the `BIGINT` wire type, which is the widest and therefore +/// the only lossless fallback. +fn lookup_int_width(widths: &[(&str, IntWidth)], column: &str) -> Option { + widths + .iter() + .find(|(name, _)| name.eq_ignore_ascii_case(column)) + .map(|(_, w)| *w) +} + fn convert_column_type(ct: &nodedb_types::columnar::ColumnType) -> SqlDataType { use nodedb_types::columnar::ColumnType; match ct { @@ -185,7 +231,14 @@ fn parse_type_str(s: &str) -> SqlDataType { return SqlDataType::Decimal; } match upper.as_str() { - "INT" | "INTEGER" | "INT4" | "INT8" | "BIGINT" | "SMALLINT" | "INT2" => SqlDataType::Int64, + // Every spelling `IntWidth::from_declared_type` recognizes must appear + // here too, or the column resolves to the `_ => String` default and + // advertises OID 25 (text) — the exact failure that made `SMALLINT` + // columns unreadable (issue #217). `parse_type_str` decides *whether* + // the column is an integer; `IntWidth` decides *how wide*. + "INT" | "INTEGER" | "INT4" | "INT8" | "INT64" | "BIGINT" | "SMALLINT" | "INT2" => { + SqlDataType::Int64 + } "FLOAT" | "FLOAT4" | "FLOAT8" | "FLOAT64" | "DOUBLE" | "REAL" => SqlDataType::Float64, "BOOL" | "BOOLEAN" => SqlDataType::Bool, "BYTES" | "BYTEA" | "BLOB" => SqlDataType::Bytes, diff --git a/nodedb/src/control/planner/sql_plan_convert/output_schema.rs b/nodedb/src/control/planner/sql_plan_convert/output_schema.rs index e4d23f501..55dd8ba09 100644 --- a/nodedb/src/control/planner/sql_plan_convert/output_schema.rs +++ b/nodedb/src/control/planner/sql_plan_convert/output_schema.rs @@ -20,7 +20,7 @@ use nodedb_sql::types_expr::SqlExpr; use super::lateral::collection_name_from_plan; use super::output_schema_types::{infer_aggregate_type, infer_computed_expr_type}; use crate::control::server::response_shape::schema::{ - OutputColumn, OutputSchema, sql_data_type_to_ddl_col_type_with_raw, + OutputColumn, OutputSchema, sql_data_type_to_ddl_col_type_with_width, }; use crate::control::server::response_shape::types::DdlColType; @@ -97,7 +97,7 @@ fn column_types_for( .map(|c| { ( c.name.clone(), - sql_data_type_to_ddl_col_type_with_raw(&c.data_type, c.raw_type.as_deref()), + sql_data_type_to_ddl_col_type_with_width(&c.data_type, c.int_width), ) }) .collect(), @@ -178,7 +178,7 @@ fn ordered_columns_for( .map(|c| OutputColumn { display_name: c.name.clone(), lookup_key: c.name.clone(), - ty: sql_data_type_to_ddl_col_type_with_raw(&c.data_type, c.raw_type.as_deref()), + ty: sql_data_type_to_ddl_col_type_with_width(&c.data_type, c.int_width), }) .collect(), _ => Vec::new(), @@ -778,6 +778,7 @@ mod tests { is_primary_key: false, default: None, raw_type: None, + int_width: None, }; Ok(Some(nodedb_sql::types::CollectionInfo { name: "metrics".to_string(), diff --git a/nodedb/src/control/server/pgwire/catalog/schema.rs b/nodedb/src/control/server/pgwire/catalog/schema.rs index 5f21bd86d..973de8178 100644 --- a/nodedb/src/control/server/pgwire/catalog/schema.rs +++ b/nodedb/src/control/server/pgwire/catalog/schema.rs @@ -219,6 +219,7 @@ pub fn catalog_collection_info(name: &str) -> Option { is_primary_key: false, default: None, raw_type: None, + int_width: None, }) .collect(); Some(CollectionInfo { diff --git a/nodedb/src/control/server/pgwire/catalog/tables/collections.rs b/nodedb/src/control/server/pgwire/catalog/tables/collections.rs index 02379b779..0bdce38d4 100644 --- a/nodedb/src/control/server/pgwire/catalog/tables/collections.rs +++ b/nodedb/src/control/server/pgwire/catalog/tables/collections.rs @@ -50,7 +50,19 @@ pub fn has_secondary_index(coll: &StoredCollection) -> bool { !coll.indexes.is_empty() } +/// Map a declared catalog type string to the PostgreSQL type OID that +/// `pg_attribute` / `\d` reports for it. +/// +/// Integer widths are resolved through +/// [`IntWidth::from_declared_type`](nodedb_types::columnar::IntWidth::from_declared_type) +/// rather than matched here, so catalog introspection and `RowDescription` +/// cannot disagree about how wide a column is — a split that previously showed +/// up as `\d` reporting OID 23 for a column `SELECT` advertised as 20. pub fn field_type_to_oid(field_type: &str) -> i64 { + if let Some(width) = nodedb_types::columnar::IntWidth::from_declared_type(field_type) { + return i64::from(width.pg_oid()); + } + let normalized = field_type.trim().to_ascii_lowercase(); let starts_with_type = |name: &str| { normalized == name @@ -72,12 +84,6 @@ pub fn field_type_to_oid(field_type: &str) -> i64 { || starts_with_type("float8") { 701 - } else if starts_with_type("integer") || starts_with_type("int4") || starts_with_type("int") { - 23 - } else if starts_with_type("smallint") || starts_with_type("int2") { - 21 - } else if starts_with_type("bigint") || starts_with_type("int8") { - 20 } else if starts_with_type("float") || starts_with_type("float4") || starts_with_type("real") { 700 } else if starts_with_type("bool") || starts_with_type("boolean") { diff --git a/nodedb/src/control/server/pgwire/handler/shape_encode.rs b/nodedb/src/control/server/pgwire/handler/shape_encode.rs index 6420e0af7..b3c8a4dc5 100644 --- a/nodedb/src/control/server/pgwire/handler/shape_encode.rs +++ b/nodedb/src/control/server/pgwire/handler/shape_encode.rs @@ -22,6 +22,7 @@ use pgwire::error::PgWireResult; use pgwire::messages::data::DataRow; use nodedb_types::NdbDateTime; +use nodedb_types::columnar::IntWidth; use crate::control::server::response_shape::project::json_value_to_text; use crate::control::server::response_shape::types::{DdlColType, ShapedRows}; @@ -89,8 +90,29 @@ fn encode_typed_cell( if format == FieldFormat::Binary { match ct { DdlColType::Int8 => return encoder.encode_field(&v.as_i64()), - DdlColType::Int4 => return encoder.encode_field(&v.as_i64().map(|n| n as i32)), - DdlColType::Int2 => return encoder.encode_field(&v.as_i64().map(|n| n as i16)), + // Narrowing casts are fallible, so they are `try_from`, not `as`. + // A stored value wider than the column's declared width cannot be + // transmitted under a narrowed OID: the client reads exactly 2 or 4 + // bytes and would silently decode a wrapped number. Writes are + // range-checked (`nodedb_sql::planner::dml`), so this is + // unreachable for data written through SQL — but rows predating the + // declared width, or arriving via a non-SQL ingest path, can still + // be out of range, and those must surface as an error rather than + // corrupt a value in flight. + DdlColType::Int4 => { + // `as i32` is lossless here: `checked_narrow` has already + // proved the value is inside `IntWidth::I32`. + return match checked_narrow(v, IntWidth::I32)? { + Some(n) => encoder.encode_field(&(n as i32)), + None => encoder.encode_field(&None::), + }; + } + DdlColType::Int2 => { + return match checked_narrow(v, IntWidth::I16)? { + Some(n) => encoder.encode_field(&(n as i16)), + None => encoder.encode_field(&None::), + }; + } DdlColType::Float8 => return encoder.encode_field(&v.as_f64()), DdlColType::Float4 => return encoder.encode_field(&v.as_f64().map(|f| f as f32)), DdlColType::Bool => return encoder.encode_field(&v.as_bool()), @@ -135,6 +157,44 @@ fn encode_typed_cell( } } +/// Range-check an integer cell against the width its column advertises, +/// before it is narrowed for binary transmission. +/// +/// `Ok(None)` means the cell is absent or not an integer and encodes as SQL +/// NULL, matching the wider `Int8` arm. `Ok(Some(n))` guarantees `n` fits +/// `width`, so the caller's narrowing cast is lossless by construction. +/// +/// An out-of-range value is a hard error rather than a truncation: the +/// column's `RowDescription` already told the client to read two or four +/// bytes, so no encoding of the true value is available here, and a wrapped +/// one is undetectable at the far end. SQLSTATE `22003` +/// (`numeric_value_out_of_range`) is what PostgreSQL raises for the same +/// condition, so drivers already classify it as a data error. +/// +/// Writes through SQL are range-checked at plan time +/// (`nodedb_sql::planner::dml_helpers::check_declared_int_ranges`), which +/// makes this unreachable for data nodedb accepted itself. It still has to +/// exist: rows written before a column's width was declared, and rows +/// arriving over non-SQL ingest paths, are not covered by that check. +fn checked_narrow(v: &serde_json::Value, width: IntWidth) -> PgWireResult> { + let Some(n) = v.as_i64() else { + return Ok(None); + }; + if width.contains(n) { + return Ok(Some(n)); + } + Err(pgwire::error::PgWireError::UserError(Box::new( + pgwire::error::ErrorInfo::new( + "ERROR".into(), + "22003".into(), + format!( + "value {n} is out of range for type {}", + width.pg_type_name() + ), + ), + ))) +} + /// Build a `Response::Query` from a protocol-neutral [`ShapedRows`], plus its /// carried client-facing notice. /// diff --git a/nodedb/src/control/server/response_shape/schema.rs b/nodedb/src/control/server/response_shape/schema.rs index 71444302d..8ab353488 100644 --- a/nodedb/src/control/server/response_shape/schema.rs +++ b/nodedb/src/control/server/response_shape/schema.rs @@ -63,68 +63,51 @@ pub fn sql_data_type_to_ddl_col_type( } } -/// Like [`sql_data_type_to_ddl_col_type`], but refines an `Int8` result using -/// the catalog's declared raw type string (`ColumnInfo::raw_type`), when -/// present. +/// Like [`sql_data_type_to_ddl_col_type`], but narrows an `Int8` result to the +/// column's declared integer width (`ColumnInfo::int_width`) when the catalog +/// recorded one. /// -/// # Why only `Int8` is refined +/// # Why only `Int8` is narrowed /// /// `SqlDataType::Int64` is the single planner-facing type for every integer -/// width — `SMALLINT`/`INT2`, `INTEGER`/`INT4`, and `BIGINT`/`INT8` all parse +/// width — `SMALLINT`/`INT2`, `INTEGER`/`INT4`, and `BIGINT`/`INT8` all resolve /// to it (see `catalog_adapter::type_convert::parse_type_str` and -/// `nodedb_types::columnar::ColumnType::from_str`), because nodedb's -/// storage — columnar, strict, and kv alike — always keeps integers as a -/// full `i64`. The declared width therefore carries no storage or -/// computation meaning; it is authoritative for exactly one thing: the wire -/// type a pgwire client expects in `RowDescription`. A client that declared -/// `SMALLINT` expects OID 21 (and, for binary-format results, a 2-byte -/// value) — silently widening every integer to `BIGINT`'s OID 20 breaks ORMs -/// and typed client libraries that trust the advertised OID (nodedb upstream -/// issue #217). So this function narrows *display width only*, strictly -/// downstream of `sql_data_type_to_ddl_col_type`'s own type mapping: every -/// other `SqlDataType` variant (`String`, `Float64`, `Bool`, ...) passes -/// through unchanged, with `raw` ignored, because there is no other -/// wire-ambiguous case to resolve. +/// `nodedb_types::columnar::ColumnType::from_str`), because nodedb's storage — +/// columnar, strict, and kv alike — always keeps integers as a full `i64`. The +/// declared width therefore carries no storage meaning; it is authoritative for +/// the wire contract: a client that declared `SMALLINT` expects OID 21 and, in +/// binary format, exactly two bytes. Silently widening every integer to +/// `BIGINT`'s OID 20 breaks ORMs and typed client libraries that trust the +/// advertised OID (issue #217). Every other `SqlDataType` variant passes +/// through unchanged with `width` ignored — there is no other wire-ambiguous +/// case to resolve. /// -/// # Matching +/// # Why this takes a resolved width rather than a type string /// -/// `raw` is matched case-insensitively by keyword, using the same -/// prefix-with-boundary logic as -/// `pgwire::catalog::tables::collections::field_type_to_oid` (the -/// already-correct reference implementation this mirrors): `BIGINT`/`INT8` -/// are checked *first* to mirror that reference implementation's ordering, -/// but the order isn't load-bearing — the boundary check itself already -/// rejects `int` as a prefix match of `int8`, since the next character -/// (`8`) is neither `(` nor whitespace. An unrecognized or absent `raw` -/// leaves the base `Int8` untouched — refinement is best-effort, never a -/// hard error. -pub fn sql_data_type_to_ddl_col_type_with_raw( +/// The declared width is also enforced on the write path, and the two must +/// agree exactly or the narrowing here would be advertising a contract writes +/// do not honour. Both sides read the same +/// [`IntWidth`](nodedb_types::columnar::IntWidth), resolved once at the catalog +/// boundary by `catalog_adapter::type_convert`, so disagreement is not +/// representable. A `None` width means the catalog has no record of the +/// declared type (for example a planner-synthesized column) and leaves the base +/// `Int8` — the widest, and so the only lossless, fallback. +pub fn sql_data_type_to_ddl_col_type_with_width( ty: &nodedb_sql::types_expr::SqlDataType, - raw: Option<&str>, + width: Option, ) -> super::types::DdlColType { use super::types::DdlColType; + use nodedb_types::columnar::IntWidth; let base = sql_data_type_to_ddl_col_type(ty); - let (DdlColType::Int8, Some(raw)) = (base, raw) else { + let (DdlColType::Int8, Some(width)) = (base, width) else { return base; }; - let normalized = raw.trim().to_ascii_lowercase(); - let starts_with_type = |name: &str| { - normalized == name - || normalized.strip_prefix(name).is_some_and(|rest| { - rest.starts_with('(') || rest.chars().next().is_some_and(char::is_whitespace) - }) - }; - - if starts_with_type("bigint") || starts_with_type("int8") { - DdlColType::Int8 - } else if starts_with_type("smallint") || starts_with_type("int2") { - DdlColType::Int2 - } else if starts_with_type("integer") || starts_with_type("int4") || starts_with_type("int") { - DdlColType::Int4 - } else { - DdlColType::Int8 + match width { + IntWidth::I16 => DdlColType::Int2, + IntWidth::I32 => DdlColType::Int4, + IntWidth::I64 => DdlColType::Int8, } } @@ -133,77 +116,63 @@ mod tests { use super::super::types::DdlColType; use super::*; use nodedb_sql::types_expr::SqlDataType; + use nodedb_types::columnar::IntWidth; - /// `sql_data_type_to_ddl_col_type_with_raw` must narrow an `Int64` - /// (base `Int8`) column to `Int4`/`Int2` when the catalog's declared raw - /// type string says the DDL author wrote a narrower integer keyword. - /// `BIGINT`/`INT8` stay `Int8`. Case-insensitive, matching every other - /// raw-type match in this codebase (e.g. `field_type_to_oid`). + /// Each declared width narrows the `Int8` base to its own wire type, and + /// the mapping matches the OIDs `IntWidth` itself advertises — locking the + /// two representations of "how wide is this column" against drift. #[test] - fn refine_narrows_int8_by_raw_integer_keyword() { - let cases: &[(&str, DdlColType)] = &[ - ("BIGINT", DdlColType::Int8), - ("INT8", DdlColType::Int8), - ("INTEGER", DdlColType::Int4), - ("INT", DdlColType::Int4), - ("INT4", DdlColType::Int4), - ("SMALLINT", DdlColType::Int2), - ("INT2", DdlColType::Int2), + fn narrows_int8_to_each_declared_width() { + let cases: &[(IntWidth, DdlColType, u32)] = &[ + (IntWidth::I16, DdlColType::Int2, 21), + (IntWidth::I32, DdlColType::Int4, 23), + (IntWidth::I64, DdlColType::Int8, 20), ]; - for (raw, expected) in cases { + for (width, expected, expected_oid) in cases { assert_eq!( - sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::Int64, Some(raw)), + sql_data_type_to_ddl_col_type_with_width(&SqlDataType::Int64, Some(*width)), *expected, - "raw type {raw:?} must refine to {expected:?}" + "declared width {width:?} must narrow to {expected:?}" ); - // Case-insensitivity. assert_eq!( - sql_data_type_to_ddl_col_type_with_raw( - &SqlDataType::Int64, - Some(&raw.to_lowercase()) - ), - *expected, - "lowercase raw type {raw:?} must refine to {expected:?}" + width.pg_oid(), + *expected_oid, + "declared width {width:?} must advertise OID {expected_oid}" ); } } - /// A raw string that isn't a recognized integer keyword leaves the base - /// `Int8` untouched — the refinement is best-effort, never a hard error. - #[test] - fn refine_unrecognized_raw_keyword_stays_int8() { - assert_eq!( - sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::Int64, Some("NOT_A_TYPE")), - DdlColType::Int8 - ); - } - - /// `raw = None` (planner-synthesized columns, e.g. an auto-injected - /// `id`, carry no raw type) passes through to the unrefined base type. + /// `width = None` — a planner-synthesized column, or one whose declared + /// type the catalog never recorded — stays at the base `Int8`. `BIGINT` is + /// the widest integer wire type, so it is the only fallback that cannot + /// truncate a stored value. #[test] - fn refine_with_no_raw_type_stays_base() { + fn no_declared_width_stays_int8() { assert_eq!( - sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::Int64, None), + sql_data_type_to_ddl_col_type_with_width(&SqlDataType::Int64, None), DdlColType::Int8 ); } - /// Only an `Int8` base is eligible for refinement — every other + /// Only an `Int8` base is eligible for narrowing — every other /// `SqlDataType` passes through `sql_data_type_to_ddl_col_type` exactly, - /// with `raw` ignored even when it names an integer keyword (a raw type - /// string can never disagree with the planner's own resolved - /// `SqlDataType`, but this proves the fn doesn't misapply integer - /// narrowing to non-integer columns regardless). + /// with `width` ignored even when one is supplied. A width can never + /// disagree with the planner's resolved `SqlDataType` in practice, but + /// this proves narrowing is never misapplied to a non-integer column. #[test] - fn refine_passes_through_non_int8_types_untouched() { + fn passes_through_non_int8_types_untouched() { assert_eq!( - sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::String, Some("SMALLINT")), + sql_data_type_to_ddl_col_type_with_width(&SqlDataType::String, Some(IntWidth::I16)), DdlColType::Text ); assert_eq!( - sql_data_type_to_ddl_col_type_with_raw(&SqlDataType::Float64, None), + sql_data_type_to_ddl_col_type_with_width(&SqlDataType::Float64, Some(IntWidth::I32)), DdlColType::Float8 ); + assert_eq!( + sql_data_type_to_ddl_col_type_with_width(&SqlDataType::Bool, None), + DdlColType::Bool + ); } #[test] diff --git a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/add_column.rs b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/add_column.rs index ff805cda2..17b4e7b96 100644 --- a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/add_column.rs +++ b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/add_column.rs @@ -31,6 +31,15 @@ pub(super) async fn alter_table_add_column( let column = parse_origin_column_def(col_def_str).map_err(|e| err("42601", e.to_string()))?; let column_name = column.name.clone(); + // The declared type as written, e.g. `SMALLINT` from `age SMALLINT NOT + // NULL`. `ColumnDef::column_type` cannot supply this: it has one `Int64` + // variant for every integer width. Falls back to the resolved type's own + // name when the definition has no separate type token to quote. + let declared_type = col_def_str + .split_whitespace() + .nth(1) + .map(str::to_string) + .unwrap_or_else(|| column.column_type.to_string()); // Validate: new column must be nullable or have a default. if !column.nullable && column.default.is_none() { @@ -67,6 +76,16 @@ pub(super) async fn alter_table_add_column( let mut updated = coll; updated.collection_type = nodedb_types::CollectionType::strict(schema.clone()); updated.timeseries_config = sonic_rs::to_string(&schema).ok(); + // Record the column's *declared* type alongside the + // resolved one — see `strict_schema::retype_field`. Without + // this the added column has no declared width and falls + // back to `BIGINT` on the wire, unlike an identical column + // declared at CREATE time. + super::strict_schema::add_field( + &mut updated, + &column_name, + declared_type.as_str(), + ); let entry = crate::control::catalog_entry::CatalogEntry::PutCollection( Box::new(updated.clone()), ); diff --git a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/alter_type.rs b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/alter_type.rs index 2424072a7..8091c6c32 100644 --- a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/alter_type.rs +++ b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/alter_type.rs @@ -16,7 +16,9 @@ use crate::control::security::identity::AuthenticatedIdentity; use crate::control::server::shared::ddl::result::{DdlError, DdlResult}; use crate::control::state::SharedState; -use super::strict_schema::{load_strict_collection, persist_schema_change, write_schema_back}; +use super::strict_schema::{ + load_strict_collection, persist_schema_change, retype_field, write_schema_back, +}; use super::support::{err, status}; /// ALTER COLLECTION ALTER COLUMN TYPE @@ -62,6 +64,9 @@ pub(super) async fn alter_collection_alter_column_type( let mut updated = coll; write_schema_back(&mut updated, schema); + // The declared spelling, not the resolved `ColumnType`, is what carries + // the integer width — see `retype_field`. + retype_field(&mut updated, column_name, new_type_str); persist_schema_change(state, &updated).await?; state.audit_record( diff --git a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/drop_column.rs b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/drop_column.rs index 996a39d3c..ffaaeb544 100644 --- a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/drop_column.rs +++ b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/drop_column.rs @@ -14,7 +14,9 @@ use crate::control::security::identity::AuthenticatedIdentity; use crate::control::server::shared::ddl::result::{DdlError, DdlResult}; use crate::control::state::SharedState; -use super::strict_schema::{load_strict_collection, persist_schema_change, write_schema_back}; +use super::strict_schema::{ + load_strict_collection, persist_schema_change, remove_field, write_schema_back, +}; use super::support::{err, status}; /// ALTER COLLECTION DROP COLUMN @@ -60,6 +62,7 @@ pub(super) async fn alter_collection_drop_column( let mut updated = coll; write_schema_back(&mut updated, schema); + remove_field(&mut updated, column_name); persist_schema_change(state, &updated).await?; state.audit_record( diff --git a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/rename_column.rs b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/rename_column.rs index f465dd545..2502b84df 100644 --- a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/rename_column.rs +++ b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/rename_column.rs @@ -14,7 +14,9 @@ use crate::control::security::identity::AuthenticatedIdentity; use crate::control::server::shared::ddl::result::{DdlError, DdlResult}; use crate::control::state::SharedState; -use super::strict_schema::{load_strict_collection, persist_schema_change, write_schema_back}; +use super::strict_schema::{ + load_strict_collection, persist_schema_change, rename_field, write_schema_back, +}; use super::support::{err, status}; /// ALTER COLLECTION RENAME COLUMN TO @@ -56,6 +58,7 @@ pub(super) async fn alter_collection_rename_column( let mut updated = coll; write_schema_back(&mut updated, schema); + rename_field(&mut updated, old_name, new_name); persist_schema_change(state, &updated).await?; state.audit_record( diff --git a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/strict_schema.rs b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/strict_schema.rs index 43fad9ca2..df4e9aba9 100644 --- a/nodedb/src/control/server/shared/ddl/neutral/collection/alter/strict_schema.rs +++ b/nodedb/src/control/server/shared/ddl/neutral/collection/alter/strict_schema.rs @@ -69,6 +69,48 @@ pub(super) fn write_schema_back( coll.timeseries_config = sonic_rs::to_string(&schema).ok(); } +/// Retype a column's entry in `coll.fields`, the catalog's record of the +/// *declared* type string each column was created with. +/// +/// `fields` is not redundant with the strict schema: `ColumnType` collapses +/// every integer width onto one `Int64` variant, so the declared spelling is +/// the only surviving record of how wide the author said the column was. That +/// spelling drives the column's advertised wire OID and the range accepted on +/// write, which is exactly why `ALTER COLUMN TYPE` — whose only supported use +/// *is* an alias change such as `INT` → `BIGINT` — has to update it. Leaving +/// it stale would make the alter a silent no-op for the case it exists to +/// serve, and would keep rejecting writes the new type allows. +pub(super) fn retype_field(coll: &mut StoredCollection, column: &str, new_type: &str) { + for (name, type_str) in coll.fields.iter_mut() { + if name.eq_ignore_ascii_case(column) { + *type_str = new_type.to_string(); + } + } +} + +/// Rename a column's entry in `coll.fields`, keeping its declared type. +pub(super) fn rename_field(coll: &mut StoredCollection, old_name: &str, new_name: &str) { + for (name, _) in coll.fields.iter_mut() { + if name.eq_ignore_ascii_case(old_name) { + *name = new_name.to_string(); + } + } +} + +/// Remove a column's entry from `coll.fields`. +pub(super) fn remove_field(coll: &mut StoredCollection, column: &str) { + coll.fields + .retain(|(name, _)| !name.eq_ignore_ascii_case(column)); +} + +/// Append a column's declared type to `coll.fields`, replacing any existing +/// entry of the same name. +pub(super) fn add_field(coll: &mut StoredCollection, column: &str, declared_type: &str) { + remove_field(coll, column); + coll.fields + .push((column.to_string(), declared_type.to_string())); +} + /// Replicate the mutated collection through the metadata raft group, /// refresh this node's Data Plane register so the in-memory shape /// catches up with the new schema, then bump `schema_version`. diff --git a/nodedb/tests/ddl_int_width_aliases_strict_kv.rs b/nodedb/tests/ddl_int_width_aliases_strict_kv.rs index 4bb3c7cb6..75e258959 100644 --- a/nodedb/tests/ddl_int_width_aliases_strict_kv.rs +++ b/nodedb/tests/ddl_int_width_aliases_strict_kv.rs @@ -1,31 +1,65 @@ // SPDX-License-Identifier: BUSL-1.1 -//! Upstream issue #223 regression: `document_strict` and `kv` engines must -//! accept `INT4`/`SMALLINT` (and the other PostgreSQL integer-width -//! keywords) as valid column types. Both engines validate declared column -//! types via `nodedb_types::columnar::ColumnType::from_str` +//! Upstream issue #223: `document_strict` and `kv` collections must accept +//! every PostgreSQL integer-width keyword in DDL *and* report each column's +//! declared width faithfully on the wire. +//! +//! Both engines validate declared column types via +//! `nodedb_types::columnar::ColumnType::from_str` //! (`nodedb-sql/src/ddl_ast/collection_type.rs`'s `build_strict_schema` / -//! `build_kv_collection_type`), which previously only recognized +//! `build_kv_collection_type`), which previously recognized only //! `BIGINT`/`INT64`/`INTEGER`/`INT` — `INT4`, `INT8`, `SMALLINT`, and `INT2` -//! were rejected with "unknown column type", even though they're valid -//! PostgreSQL integer aliases. +//! were rejected as "unknown column type". +//! +//! Accepting those spellings is only half the fix. `ColumnType` deliberately +//! has one `Int64` variant for every declared width (nodedb stores all +//! integers as a full i64), so a strict/kv column's resolved type cannot say +//! how wide the author declared it. The declared width is recovered from the +//! catalog's raw `fields` entries — populated for strict and kv exactly as for +//! schemaless and columnar — and resolved to an `IntWidth` at the catalog +//! boundary. Without that, `CREATE ... (a SMALLINT)` would succeed and then +//! report OID 20, trading a loud DDL error for a silent width mismatch. //! -//! This is a DDL-acceptance smoke test only: for these engines the wire OID -//! stays `INT8` (20) regardless of declared width — `raw_type` refinement -//! (issue #217) is schemaless/columnar-only by design, since strict/kv -//! columns are typed authoritatively by the resolved `ColumnType`, not a -//! separately-tracked raw string. See `pgwire_ddl_result_types.rs`'s -//! `create_collection_int_widths_preserve_wire_oids` for the schemaless OID -//! fidelity coverage. +//! Companion coverage: `pgwire_ddl_result_types.rs` for schemaless OID +//! fidelity, and `pgwire_int_width_range_enforcement.rs` for the write-side +//! range constraint that makes these narrowed OIDs honest. mod common; use common::pgwire_harness::TestServer; -/// `document_strict` `CREATE COLLECTION` with `INT4`/`SMALLINT` columns must -/// succeed (previously errored with "unknown column type: 'INT4'"). +/// Assert the exact `RowDescription` OID of each named column, failing loudly +/// if a column is missing entirely. +/// +/// Deliberately not `if let Some(col)` — a lookup that silently skips turns +/// this into a test that passes when the columns vanish, which is precisely +/// the regression it exists to catch. +fn assert_column_oids(row: &tokio_postgres::Row, expected: &[(&str, u32)]) { + for (col_name, expected_oid) in expected { + let col = row + .columns() + .iter() + .find(|c| c.name() == *col_name) + .unwrap_or_else(|| { + panic!( + "column '{col_name}' must appear in RowDescription; got {:?}", + row.columns().iter().map(|c| c.name()).collect::>() + ) + }); + assert_eq!( + col.type_().oid(), + *expected_oid, + "column '{col_name}' must advertise OID {expected_oid}, got {}", + col.type_().oid() + ); + } +} + +/// `document_strict` `CREATE COLLECTION` with every integer-width spelling +/// must succeed (previously "unknown column type: 'INT4'") and each column +/// must advertise its own declared width. #[tokio::test(flavor = "multi_thread", worker_threads = 4)] -async fn strict_create_collection_accepts_int_width_aliases() { +async fn strict_create_collection_preserves_declared_int_widths() { let server = TestServer::start().await; server @@ -61,27 +95,23 @@ async fn strict_create_collection_accepts_int_width_aliases() { .expect("execute strict_int_widths select"); assert_eq!(rows.len(), 1, "expected 1 row back from strict_int_widths"); - // Load-bearing: document_strict stays INT8 (20) regardless of declared - // width — strict columns carry no `raw_type` (they're already typed - // authoritatively by `ColumnType`), so the issue #217 refinement never - // applies here. Width narrowing is schemaless/columnar-only by design. - for col_name in ["a", "b", "c", "d"] { - if let Some(col) = rows[0].columns().iter().find(|c| c.name() == col_name) { - assert_eq!( - col.type_().oid(), - 20, - "strict column '{col_name}' OID must stay INT8 (20), got {}", - col.type_().oid() - ); - } - } + assert_column_oids( + &rows[0], + &[("a", 23), ("b", 21), ("c", 21), ("d", 20), ("id", 25)], + ); + + // Typed getters matching the advertised widths: a wrong OID or a + // wrong-width binary payload panics inside `get` before the comparison. + assert_eq!(rows[0].get::<_, i32>("a"), 1); + assert_eq!(rows[0].get::<_, i16>("b"), 2); + assert_eq!(rows[0].get::<_, i16>("c"), 3); + assert_eq!(rows[0].get::<_, i64>("d"), 4); } -/// `kv` `CREATE COLLECTION` with `INT4`/`SMALLINT` columns must succeed -/// (previously errored with "unknown column type: 'INT4'"). The kv OID -/// stays INT8 (20) — do not assert a narrowed OID here. +/// `kv` `CREATE COLLECTION` with `INT4`/`SMALLINT` columns must succeed and +/// report each declared width, exactly as strict does. #[tokio::test(flavor = "multi_thread", worker_threads = 4)] -async fn kv_create_collection_accepts_int_width_aliases() { +async fn kv_create_collection_preserves_declared_int_widths() { let server = TestServer::start().await; server @@ -115,16 +145,68 @@ async fn kv_create_collection_accepts_int_width_aliases() { .expect("execute kv_int_widths select"); assert_eq!(rows.len(), 1, "expected 1 row back from kv_int_widths"); - // Load-bearing: kv stays INT8 (20) — the fix does not widen/narrow kv's - // wire type, only schemaless/columnar's. - for col_name in ["a", "b"] { - if let Some(col) = rows[0].columns().iter().find(|c| c.name() == col_name) { - assert_eq!( - col.type_().oid(), - 20, - "kv column '{col_name}' OID must stay INT8 (20), got {}", - col.type_().oid() - ); - } - } + assert_column_oids(&rows[0], &[("a", 23), ("b", 21)]); + assert_eq!(rows[0].get::<_, i32>("a"), 1); + assert_eq!(rows[0].get::<_, i16>("b"), 2); +} + +/// A column whose declared type carries no width (`BIGINT`) stays at OID 20, +/// and a non-integer column is untouched by width resolution — the narrowing +/// must apply to declared narrow integers only, never by default. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn undeclared_and_non_integer_widths_are_untouched() { + let server = TestServer::start().await; + + server + .exec( + "CREATE COLLECTION strict_mixed_widths (\ + id TEXT PRIMARY KEY, \ + big BIGINT, \ + label TEXT, \ + ratio DOUBLE, \ + flag BOOL\ + ) WITH (engine='document_strict')", + ) + .await + .expect("CREATE COLLECTION strict_mixed_widths must succeed"); + + server + .exec( + "INSERT INTO strict_mixed_widths (id, big, label, ratio, flag) \ + VALUES ('r1', 9876543210, 'x', 1.5, true)", + ) + .await + .expect("INSERT into strict_mixed_widths must succeed"); + + let stmt = server + .client + .prepare_typed( + "SELECT id, big, label, ratio, flag FROM strict_mixed_widths WHERE id = $1", + &[tokio_postgres::types::Type::TEXT], + ) + .await + .expect("prepare strict_mixed_widths select"); + let rows = server + .client + .query(&stmt, &[&"r1"]) + .await + .expect("execute strict_mixed_widths select"); + assert_eq!( + rows.len(), + 1, + "expected 1 row back from strict_mixed_widths" + ); + + assert_column_oids( + &rows[0], + &[ + ("big", 20), + ("label", 25), + ("ratio", 701), + ("flag", 16), + ("id", 25), + ], + ); + // A BIGINT column must still carry values no narrower type could hold. + assert_eq!(rows[0].get::<_, i64>("big"), 9876543210); } diff --git a/nodedb/tests/pgwire_extended_query.rs b/nodedb/tests/pgwire_extended_query.rs index de8782c35..3d254ca1e 100644 --- a/nodedb/tests/pgwire_extended_query.rs +++ b/nodedb/tests/pgwire_extended_query.rs @@ -544,8 +544,10 @@ async fn extended_query_binary_typed_columns_decode() { .expect("prepared typed query should succeed"); assert_eq!(rows.len(), 1); - // int8 -> i64, float8 -> f64, bool -> bool, text -> String. - let n: i64 = rows[0].get("n"); + // `n` is declared INT (the INT4 alias), so it advertises OID 23 and + // decodes as i32 — issue #217; before the fix every declared integer + // width collapsed to int8. float8 -> f64, bool -> bool, text -> String. + let n: i32 = rows[0].get("n"); let amt: f64 = rows[0].get("amt"); let flag: bool = rows[0].get("flag"); let name: &str = rows[0].get("name"); @@ -575,14 +577,15 @@ async fn extended_query_timestamp_text_fallback_with_binary_sibling() { .unwrap(); // Extended path: the query carrying a timestamp column must succeed, and - // the int8 sibling decodes from binary. + // the integer sibling decodes from binary. `n` is declared INT, so it + // advertises OID 23 and decodes as i32 (issue #217). let rows = server .client .query("SELECT n, ts FROM ev WHERE id = $1", &[&"a"]) .await .expect("query with a timestamp column should succeed"); assert_eq!(rows.len(), 1); - let n: i64 = rows[0].get("n"); + let n: i32 = rows[0].get("n"); assert_eq!(n, 7); // Simple-query path returns every column as text, including the timestamp. diff --git a/nodedb/tests/pgwire_extended_query_engines.rs b/nodedb/tests/pgwire_extended_query_engines.rs index 991f440b0..0bbd60a8e 100644 --- a/nodedb/tests/pgwire_extended_query_engines.rs +++ b/nodedb/tests/pgwire_extended_query_engines.rs @@ -76,10 +76,14 @@ async fn extended_query_kv_integer_value() { .iter() .find(|c| c.name() == "score") .expect("score column must appear in RowDescription"); + // `score` is declared `INT`, the INT4 alias — issue #217 fixed this column + // advertising OID 20 (int8) for every declared integer width. KV resolves + // declared widths through the same catalog path as every other engine, so + // it now correctly narrows to OID 23 (int4). assert_eq!( col_score.type_().oid(), - 20, - "KV INT column 'score' must have OID 20 (int8), got {}", + 23, + "KV INT column 'score' must have OID 23 (int4), got {}", col_score.type_().oid() ); diff --git a/nodedb/tests/pgwire_int_width_range_enforcement.rs b/nodedb/tests/pgwire_int_width_range_enforcement.rs new file mode 100644 index 000000000..bbf2c5d42 --- /dev/null +++ b/nodedb/tests/pgwire_int_width_range_enforcement.rs @@ -0,0 +1,345 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Declared integer widths are enforced on write, so a narrowed +//! `RowDescription` OID is a promise the data can keep. +//! +//! Advertising OID 23 for a column declared `INT` tells a pgwire client to +//! decode exactly four bytes. nodedb stores integers as a full `i64`, so +//! without a write-side constraint a value like `9876543210` could be stored +//! in that column and then either truncated to `-1294967296` on the wire (in +//! binary format, undetectably) or fail to parse client-side (in text format). +//! +//! The constraint closes that gap at the point the value enters, exactly as +//! PostgreSQL does (`value out of range for type integer`). These tests pin +//! both halves: the write is refused, and values that *are* accepted survive a +//! binary round-trip at the declared width. + +mod common; + +use common::pgwire_harness::TestServer; + +/// The narrowest value that does not fit each declared width. +const OVER_I32: i64 = i32::MAX as i64 + 1; +const UNDER_I32: i64 = i32::MIN as i64 - 1; +const OVER_I16: i64 = i16::MAX as i64 + 1; +const UNDER_I16: i64 = i16::MIN as i64 - 1; + +async fn server_with_widths(collection: &str, engine_clause: &str) -> TestServer { + let server = TestServer::start().await; + server + .exec(&format!( + "CREATE COLLECTION {collection} (\ + id TEXT PRIMARY KEY, \ + small SMALLINT, \ + med INT, \ + big BIGINT\ + ){engine_clause}" + )) + .await + .unwrap_or_else(|e| panic!("CREATE COLLECTION {collection} must succeed: {e}")); + server +} + +/// A value past the declared width is refused, on every declared narrow +/// column and at both ends of the range. The row must not be stored. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn insert_beyond_declared_width_is_rejected() { + let server = server_with_widths("width_reject", "").await; + + let cases: &[(&str, i64)] = &[ + ("med", OVER_I32), + ("med", UNDER_I32), + ("small", OVER_I16), + ("small", UNDER_I16), + ]; + for (idx, (column, value)) in cases.iter().enumerate() { + let err = server + .exec(&format!( + "INSERT INTO width_reject (id, {column}) VALUES ('r{idx}', {value})" + )) + .await + .expect_err( + "INSERT of a value past the column's declared width must be \ + rejected, not silently stored and truncated on read", + ); + let msg = err.to_string(); + assert!( + msg.contains("out of range"), + "rejection for {column}={value} must say the value is out of \ + range; got: {msg}" + ); + } + + // Nothing from the rejected statements may have landed. + let rows = server + .client + .query("SELECT id FROM width_reject", &[]) + .await + .expect("scan width_reject"); + assert!( + rows.is_empty(), + "no rejected row may be stored; found {} row(s)", + rows.len() + ); +} + +/// The boundary values themselves are accepted — the check must reject only +/// what genuinely does not fit, not shrink the usable range by one. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn boundary_values_are_accepted_and_round_trip() { + let server = server_with_widths("width_boundary", "").await; + + server + .exec(&format!( + "INSERT INTO width_boundary (id, small, med, big) \ + VALUES ('max', {}, {}, {})", + i16::MAX, + i32::MAX, + i64::MAX + )) + .await + .expect("INSERT of each width's maximum must succeed"); + server + .exec(&format!( + "INSERT INTO width_boundary (id, small, med, big) \ + VALUES ('min', {}, {}, {})", + i16::MIN, + i32::MIN, + i64::MIN + )) + .await + .expect("INSERT of each width's minimum must succeed"); + + let stmt = server + .client + .prepare_typed( + "SELECT small, med, big FROM width_boundary WHERE id = $1", + &[tokio_postgres::types::Type::TEXT], + ) + .await + .expect("prepare width_boundary select"); + + for (id, small, med, big) in [ + ("max", i16::MAX, i32::MAX, i64::MAX), + ("min", i16::MIN, i32::MIN, i64::MIN), + ] { + let rows = server + .client + .query(&stmt, &[&id]) + .await + .unwrap_or_else(|e| panic!("execute width_boundary select for {id}: {e}")); + assert_eq!(rows.len(), 1, "expected 1 row for id={id}"); + // Typed getters at the advertised widths: these decode the binary + // payload as i16/i32/i64, so a wrapped value fails here. + assert_eq!(rows[0].get::<_, i16>("small"), small, "small for id={id}"); + assert_eq!(rows[0].get::<_, i32>("med"), med, "med for id={id}"); + assert_eq!(rows[0].get::<_, i64>("big"), big, "big for id={id}"); + } +} + +/// A bound parameter cannot smuggle an out-of-range value past the declared +/// width. +/// +/// This is the case the planner check exists for. An inline literal could in +/// principle be rejected by the parser; a parameter arrives as an `int8` on +/// the wire and is bound into the AST *before* planning, so the only thing +/// standing between it and storage is the range check. +/// +/// The parameter is declared `TEXT` with an explicit `::INT` cast rather than +/// `INT8`: inference for an `INSERT ... VALUES ($1)` placeholder reports +/// `Unknown` (which no driver will serialize an integer into), and the bind +/// layer decodes parameters from their text form, so a `TEXT` parameter is the +/// shape that reaches the planner as a bound integer. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn bound_parameter_beyond_declared_width_is_rejected() { + let server = server_with_widths("width_param", "").await; + + let stmt = server + .client + .prepare_typed( + "INSERT INTO width_param (id, med) VALUES ('p1', $1::INT)", + &[tokio_postgres::types::Type::TEXT], + ) + .await + .expect("prepare width_param insert"); + let err = server + .client + .execute(&stmt, &[&OVER_I32.to_string()]) + .await + .expect_err("a bound parameter past the declared width must be rejected"); + // `tokio_postgres::Error`'s Display is just "db error"; the server's + // message is only on the wrapped DbError. + let msg = err + .as_db_error() + .map(|e| e.message().to_string()) + .unwrap_or_else(|| err.to_string()); + assert!( + msg.contains("out of range"), + "bound-parameter rejection must come from the range check and say out \ + of range; got: {msg}" + ); + + // The same parameter inside the range is accepted, proving the rejection + // above is the width constraint and not a blanket parameter failure. + let stmt_ok = server + .client + .prepare_typed( + "INSERT INTO width_param (id, med) VALUES ('p2', $1::INT)", + &[tokio_postgres::types::Type::TEXT], + ) + .await + .expect("prepare in-range width_param insert"); + server + .client + .execute(&stmt_ok, &[&i32::MAX.to_string()]) + .await + .expect("an in-range bound parameter must be accepted"); + + let rows = server + .client + .query("SELECT id FROM width_param", &[]) + .await + .expect("scan width_param"); + let ids: Vec<&str> = rows.iter().map(|r| r.get("id")).collect(); + assert_eq!( + ids, + vec!["p2"], + "only the in-range parameter may have been stored" + ); +} + +/// `UPDATE ... SET col = ` is checked too — a write that widens an +/// existing row past its declared width is the same violation as an insert. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn update_beyond_declared_width_is_rejected() { + let server = server_with_widths("width_update", "").await; + + server + .exec("INSERT INTO width_update (id, med) VALUES ('u1', 1)") + .await + .expect("seed row must insert"); + + let err = server + .exec(&format!( + "UPDATE width_update SET med = {OVER_I32} WHERE id = 'u1'" + )) + .await + .expect_err("UPDATE past the declared width must be rejected"); + assert!( + err.to_string().contains("out of range"), + "UPDATE rejection must say out of range; got: {err}" + ); + + let rows = server + .client + .query("SELECT med FROM width_update WHERE id = 'u1'", &[]) + .await + .expect("scan width_update"); + assert_eq!(rows.len(), 1); + assert_eq!( + rows[0].get::<_, i32>("med"), + 1, + "the rejected UPDATE must leave the original value intact" + ); +} + +/// `ALTER COLUMN ... TYPE` moves the constraint with the declared type. +/// +/// Widening `INT` to `BIGINT` is the *only* thing that alter supports (a +/// cross-type change needs a rewrite and is refused), so if the catalog's +/// record of the declared type did not move with it, the alter would be a +/// silent no-op: the column would keep advertising OID 23 and keep rejecting +/// the very values the widening was performed to allow. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn altering_the_declared_type_moves_the_constraint() { + let server = TestServer::start().await; + server + .exec( + "CREATE COLLECTION width_alter (id TEXT PRIMARY KEY, n INT NOT NULL) \ + WITH (engine='document_strict')", + ) + .await + .expect("CREATE COLLECTION width_alter must succeed"); + + // Before the widening the value is refused and the column reports int4. + server + .exec(&format!( + "INSERT INTO width_alter (id, n) VALUES ('pre', {OVER_I32})" + )) + .await + .expect_err("the value must not fit the column's original INT width"); + let before = server + .client + .prepare("SELECT n FROM width_alter LIMIT 0") + .await + .expect("prepare pre-alter describe"); + assert_eq!( + before.columns()[0].type_().oid(), + 23, + "an INT column must advertise int4 before the widening" + ); + + server + .exec("ALTER COLLECTION width_alter ALTER COLUMN n TYPE BIGINT") + .await + .expect("widening INT to BIGINT must succeed"); + + // After it, the same value is accepted and the column reports int8. + server + .exec(&format!( + "INSERT INTO width_alter (id, n) VALUES ('post', {OVER_I32})" + )) + .await + .expect("the widened column must accept a value that needs BIGINT"); + let after = server + .client + .prepare("SELECT n FROM width_alter LIMIT 0") + .await + .expect("prepare post-alter describe"); + assert_eq!( + after.columns()[0].type_().oid(), + 20, + "a widened BIGINT column must advertise int8" + ); + + let rows = server + .client + .query("SELECT n FROM width_alter WHERE id = 'post'", &[]) + .await + .expect("scan width_alter"); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].get::<_, i64>("n"), OVER_I32); +} + +/// Strict and kv resolve declared widths through the same catalog path as +/// schemaless, so the constraint must hold there too rather than being a +/// schemaless-only behavior. +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn declared_width_is_enforced_on_strict_and_kv() { + let strict = server_with_widths("strict_width", " WITH (engine='document_strict')").await; + let err = strict + .exec(&format!( + "INSERT INTO strict_width (id, med) VALUES ('s1', {OVER_I32})" + )) + .await + .expect_err("document_strict must enforce the declared width"); + assert!( + err.to_string().contains("out of range"), + "strict rejection must say out of range; got: {err}" + ); + + let kv = TestServer::start().await; + kv.exec("CREATE COLLECTION kv_width (key TEXT PRIMARY KEY, med INT) WITH (engine='kv')") + .await + .expect("CREATE COLLECTION kv_width must succeed"); + let err = kv + .exec(&format!( + "INSERT INTO kv_width (key, med) VALUES ('k1', {OVER_I32})" + )) + .await + .expect_err("kv must enforce the declared width"); + assert!( + err.to_string().contains("out of range"), + "kv rejection must say out of range; got: {err}" + ); +}