From f31905acaf63deb4a51eac64eaf49da1f252b630 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Sun, 20 Sep 2026 21:50:50 +0800 Subject: [PATCH 1/2] fix(gateway): keep the routed error's SQLSTATE class GatewayErrorMap::to_pgwire restated 14 of the direct mapper's arms and sent every variant beyond them to XX000. The direct table classifies 26 of those, so a routed query reported an internal fault where the direct path reported the real class: constraint 23505, rate and quota rejections, undefined column/function/object, write conflict, memory exhaustion, fan-out, and the rest. The mapper now delegates to error_to_sqlstate, and the agreement test pins the contract for 24 of them. --- .../src/control/gateway/error_map/pgwire.rs | 158 +++++++++++++----- 1 file changed, 115 insertions(+), 43 deletions(-) diff --git a/nodedb/src/control/gateway/error_map/pgwire.rs b/nodedb/src/control/gateway/error_map/pgwire.rs index 6c14364c2..9a0b4b5b3 100644 --- a/nodedb/src/control/gateway/error_map/pgwire.rs +++ b/nodedb/src/control/gateway/error_map/pgwire.rs @@ -2,56 +2,21 @@ //! pgwire error shape: `(SQLSTATE, message)`. -use nodedb_types::error::sqlstate; - use super::gateway_map::GatewayErrorMap; use crate::Error; impl GatewayErrorMap { /// Map a gateway error into `(sqlstate, message)` for pgwire. /// - /// Returns a `'static` SQLSTATE string and an owned message string. - /// The SQLSTATE codes match those in `pgwire::types::error_to_sqlstate` - /// so migrated call-sites are wire-compatible with the old forwarding path. + /// One table answers for the direct and the routed path. This mapper's 14 + /// arms restated the direct table, but its own catch-all sent the 26 + /// variants the direct table classifies and it did not list to `XX000`. + /// A client must not see a different class because a statement crossed the + /// gateway. pub fn to_pgwire(err: &Error) -> (&'static str, String) { - match err { - Error::NotLeader { leader_addr, .. } => ( - sqlstate::DATABASE_DROPPED, - format!("cluster in leader election; leader hint: {leader_addr}"), - ), - Error::DeadlineExceeded { .. } => (sqlstate::QUERY_CANCELED, err.to_string()), - Error::RetryableSchemaChanged { descriptor } => ( - sqlstate::INTERNAL_ERROR, - format!("schema changed during execution ({descriptor}); please retry"), - ), - Error::CollectionNotFound { collection, .. } => ( - sqlstate::UNDEFINED_TABLE, - format!("collection \"{collection}\" does not exist"), - ), - Error::RejectedAuthz { .. } => (sqlstate::INSUFFICIENT_PRIVILEGE, err.to_string()), - Error::BadRequest { detail } => (sqlstate::SYNTAX_ERROR, detail.clone()), - Error::PlanError { detail } => (sqlstate::SYNTAX_ERROR, detail.clone()), - Error::Serialization { .. } | Error::Codec { .. } => { - (sqlstate::INTERNAL_ERROR, err.to_string()) - } - Error::Internal { .. } => (sqlstate::INTERNAL_ERROR, err.to_string()), - Error::NoLeader { .. } => (sqlstate::LOCK_NOT_AVAILABLE, err.to_string()), - Error::CrossCollectionNotColocated { .. } => { - (sqlstate::FEATURE_NOT_SUPPORTED, err.to_string()) - } - Error::RemoteTyped { code, message } => ( - crate::control::server::pgwire::types::error_map::numeric_code_to_sqlstate(*code), - message.clone(), - ), - // A shard verdict that rode back as a typed code keeps the exact - // SQLSTATE and message the direct dispatch path would have given. - Error::DataPlane(code) => { - let (_severity, state, message) = - crate::control::server::shared::ddl::sqlstate::error_code_to_sqlstate(code); - (state, message) - } - _ => (sqlstate::INTERNAL_ERROR, err.to_string()), - } + let (_severity, state, message) = + crate::control::server::pgwire::types::error_to_sqlstate(err); + (state, message) } } @@ -61,6 +26,7 @@ mod tests { authz, deadline, internal, not_found, not_leader, schema_changed, serialization, }; use super::*; + use nodedb_types::error::sqlstate; #[test] fn pgwire_not_leader() { @@ -74,6 +40,8 @@ mod tests { assert_eq!(code, sqlstate::QUERY_CANCELED); } + /// The gateway previously stated this arm itself; both paths now answer + /// `INTERNAL_ERROR` from the shared table, with the variant's own message. #[test] fn pgwire_schema_changed() { let (code, msg) = GatewayErrorMap::to_pgwire(&schema_changed()); @@ -105,4 +73,108 @@ mod tests { let (code, _) = GatewayErrorMap::to_pgwire(&serialization()); assert_eq!(code, sqlstate::INTERNAL_ERROR); } + + /// A routed fault keeps the class the direct path gives it. The gateway's + /// own arms restated the direct table, but the 26 variants the direct table + /// classifies and the gateway did not list reached the client as XX000. + #[test] + fn gateway_and_direct_mapper_agree_on_classified_errors() { + use crate::types::{DatabaseId, TenantId}; + + let samples = vec![ + Error::RejectedConstraint { + collection: "orders".into(), + constraint: "unique".into(), + detail: "duplicate key".into(), + }, + Error::TxnOverlayMemoryExceeded { limit: 1 << 20 }, + Error::UndefinedFunction { + name: "no_such_fn".into(), + }, + Error::UndefinedObject { + kind: "sequence", + name: "s".into(), + }, + Error::ObjectNotInPrerequisiteState { + object: "s".into(), + detail: "currval before nextval".into(), + }, + Error::UndefinedColumn { + column: "nope".into(), + }, + Error::AmbiguousColumn { + column: "id".into(), + }, + Error::UnknownStrictField { + collection: "c".into(), + column: "x".into(), + }, + Error::DivisionByZero, + Error::InvalidLimitValue { + clause: "LIMIT", + value: "-1".into(), + }, + Error::DocumentNotFound { + collection: "c".into(), + document_id: "d".into(), + }, + Error::ConflictRetry { + collection: "c".into(), + document_id: "d".into(), + }, + Error::CalvinSerializationConflict, + Error::CalvinParticipantError, + Error::SourceFrozen { + database_id: DatabaseId::new(7), + }, + Error::CloneWriteRequiresMaterialize { + collection: "c".into(), + engine: "kv".into(), + database: "db".into(), + reason: "shadowed", + }, + Error::RateExceeded { + gate: "write".into(), + detail: "over budget".into(), + retry_after_ms: 50, + }, + Error::MemoryExhausted { + engine: "kv".into(), + }, + Error::FanOutExceeded { + shards_touched: 9, + limit: 8, + }, + Error::MaterializedSumTargetNotFound { + target_collection: "t".into(), + join_column: "k".into(), + join_value: "1".into(), + }, + Error::CollectionDeactivated { + tenant_id: TenantId::new(0), + collection: "c".into(), + retention_expires_at_ns: 1, + }, + Error::FeatureNotSupported { + detail: "sparse".into(), + }, + Error::BackupTenantMismatch { + expected: 1, + actual: 2, + }, + Error::BackupKeyMismatch, + ]; + + for err in samples { + let (_sev, direct, _msg) = + crate::control::server::pgwire::types::error_to_sqlstate(&err); + let (gateway, _) = GatewayErrorMap::to_pgwire(&err); + assert_ne!( + direct, + sqlstate::INTERNAL_ERROR, + "sample has no class of its own: {err:?}" + ); + assert_eq!(gateway, direct, "routed class differs for {err:?}"); + } + } } From d854b04a21185235e0f6dd675b2b659fd41cdd8f Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Mon, 21 Sep 2026 13:48:22 +0800 Subject: [PATCH 2/2] fix(gateway): render the schema-change wording from the variant's Display The delegation moved this condition's message to the direct table's Display text, and three cluster tests key on 'schema changed during execution'. The wording now lives in the #[error(...)] attribute so every path renders it from Display: the HTTP, RESP, and native gateway maps, the classify table, the crash harness, and the descriptor-lease integration test all read that same text. The gateway comments state the invariant (one table answers both paths) instead of the pre-change layout. --- .../descriptor_lease_planner_integration.rs | 12 ++++++------ nodedb/src/control/gateway/error_map/http.rs | 5 +---- nodedb/src/control/gateway/error_map/native.rs | 5 +---- nodedb/src/control/gateway/error_map/pgwire.rs | 16 ++++++---------- nodedb/src/control/gateway/error_map/resp.rs | 4 +--- nodedb/src/error/types.rs | 4 ++-- nodedb/src/error_classify.rs | 4 +--- nodedb/tests/crash_harness/pgwire.rs | 18 +++++++----------- 8 files changed, 25 insertions(+), 43 deletions(-) diff --git a/nodedb-cluster-tests/tests/common_suite/cases/descriptor_lease_planner_integration.rs b/nodedb-cluster-tests/tests/common_suite/cases/descriptor_lease_planner_integration.rs index 5c192a199..0ab45f660 100644 --- a/nodedb-cluster-tests/tests/common_suite/cases/descriptor_lease_planner_integration.rs +++ b/nodedb-cluster-tests/tests/common_suite/cases/descriptor_lease_planner_integration.rs @@ -115,16 +115,16 @@ async fn drain_forces_plan_retry_surfacing_typed_error() { }) .await; - // A SELECT should fail fast: the pgwire retry helper makes - // 3 attempts, each hits the drain check, each surfaces - // `RetryableSchemaChanged`. After the retry budget the - // error maps to a plan error visible to the client. + // A SELECT must fail fast: the pgwire retry helper makes 3 attempts, + // each hits the drain check and surfaces `RetryableSchemaChanged`; after + // the retry budget the client reads that variant's Display text. let result = leader.exec("SELECT * FROM retry_me").await; assert!(result.is_err(), "SELECT must fail while drain is active"); let err = result.unwrap_err(); assert!( - err.to_lowercase().contains("retryable schema change") - || err.to_lowercase().contains("plan error"), + err.to_lowercase() + .contains("schema changed during execution") + || err.to_lowercase().contains("please retry"), "unexpected error variant: {err}" ); diff --git a/nodedb/src/control/gateway/error_map/http.rs b/nodedb/src/control/gateway/error_map/http.rs index 62316dde3..281e31435 100644 --- a/nodedb/src/control/gateway/error_map/http.rs +++ b/nodedb/src/control/gateway/error_map/http.rs @@ -23,10 +23,7 @@ impl GatewayErrorMap { format!("cluster in leader election; leader hint: {leader_addr}"), ), Error::DeadlineExceeded { .. } => (504, err.to_string()), - Error::RetryableSchemaChanged { descriptor } => ( - 503, - format!("schema changed during execution ({descriptor}); please retry"), - ), + Error::RetryableSchemaChanged { .. } => (503, err.to_string()), Error::CollectionNotFound { collection, .. } => { (404, format!("collection \"{collection}\" does not exist")) } diff --git a/nodedb/src/control/gateway/error_map/native.rs b/nodedb/src/control/gateway/error_map/native.rs index 14ec740ac..f9686cb90 100644 --- a/nodedb/src/control/gateway/error_map/native.rs +++ b/nodedb/src/control/gateway/error_map/native.rs @@ -26,10 +26,7 @@ impl GatewayErrorMap { (CODE_NOT_LEADER, format!("not leader; hint: {leader_addr}")) } Error::DeadlineExceeded { .. } => (CODE_DEADLINE, err.to_string()), - Error::RetryableSchemaChanged { descriptor } => ( - CODE_SCHEMA_CHANGED, - format!("schema changed ({descriptor})"), - ), + Error::RetryableSchemaChanged { .. } => (CODE_SCHEMA_CHANGED, err.to_string()), Error::CollectionNotFound { collection, .. } => ( CODE_NOT_FOUND, format!("collection \"{collection}\" not found"), diff --git a/nodedb/src/control/gateway/error_map/pgwire.rs b/nodedb/src/control/gateway/error_map/pgwire.rs index 9a0b4b5b3..6c69cb6dc 100644 --- a/nodedb/src/control/gateway/error_map/pgwire.rs +++ b/nodedb/src/control/gateway/error_map/pgwire.rs @@ -8,11 +8,8 @@ use crate::Error; impl GatewayErrorMap { /// Map a gateway error into `(sqlstate, message)` for pgwire. /// - /// One table answers for the direct and the routed path. This mapper's 14 - /// arms restated the direct table, but its own catch-all sent the 26 - /// variants the direct table classifies and it did not list to `XX000`. - /// A client must not see a different class because a statement crossed the - /// gateway. + /// One table answers for the direct and the routed path, so a client sees + /// the same class and message for a given error either way. pub fn to_pgwire(err: &Error) -> (&'static str, String) { let (_severity, state, message) = crate::control::server::pgwire::types::error_to_sqlstate(err); @@ -40,8 +37,8 @@ mod tests { assert_eq!(code, sqlstate::QUERY_CANCELED); } - /// The gateway previously stated this arm itself; both paths now answer - /// `INTERNAL_ERROR` from the shared table, with the variant's own message. + /// Both paths answer `INTERNAL_ERROR` from the shared table, with the + /// variant's own message naming the descriptor. #[test] fn pgwire_schema_changed() { let (code, msg) = GatewayErrorMap::to_pgwire(&schema_changed()); @@ -74,9 +71,8 @@ mod tests { assert_eq!(code, sqlstate::INTERNAL_ERROR); } - /// A routed fault keeps the class the direct path gives it. The gateway's - /// own arms restated the direct table, but the 26 variants the direct table - /// classifies and the gateway did not list reached the client as XX000. + /// For every classified variant, the routed class equals the direct class, + /// and the direct class is not `XX000`. #[test] fn gateway_and_direct_mapper_agree_on_classified_errors() { use crate::types::{DatabaseId, TenantId}; diff --git a/nodedb/src/control/gateway/error_map/resp.rs b/nodedb/src/control/gateway/error_map/resp.rs index 97e479900..0e30c1a0b 100644 --- a/nodedb/src/control/gateway/error_map/resp.rs +++ b/nodedb/src/control/gateway/error_map/resp.rs @@ -26,9 +26,7 @@ impl GatewayErrorMap { } Error::RejectedConstraint { detail, .. } => format!("CONSTRAINT {detail}"), Error::TypeMismatch { detail, .. } => format!("WRONGTYPE {detail}"), - Error::RetryableSchemaChanged { descriptor } => { - format!("ERR schema changed ({descriptor}); please retry") - } + Error::RetryableSchemaChanged { .. } => format!("ERR {err}"), Error::RemoteTyped { code, message } => { format!("{} {message}", remote_code_to_resp_prefix(*code)) } diff --git a/nodedb/src/error/types.rs b/nodedb/src/error/types.rs index 68c3c910b..2d385b35b 100644 --- a/nodedb/src/error/types.rs +++ b/nodedb/src/error/types.rs @@ -346,8 +346,8 @@ pub enum Error { #[error("invalid {clause} value: {value}")] InvalidLimitValue { clause: &'static str, value: String }, - /// Descriptor lease conflict; pgwire retries within `PLAN_RETRY_BUDGET`. - #[error("retryable schema change on {descriptor}")] + /// Retried within `PLAN_RETRY_BUDGET`; every path renders this wording from `Display`. + #[error("schema changed during execution ({descriptor}); please retry")] RetryableSchemaChanged { descriptor: String }, /// Leader-change overwrite; callers must re-propose to avoid false success. diff --git a/nodedb/src/error_classify.rs b/nodedb/src/error_classify.rs index afc808472..11d5fe5f5 100644 --- a/nodedb/src/error_classify.rs +++ b/nodedb/src/error_classify.rs @@ -177,9 +177,7 @@ pub(crate) fn classify(e: &Error) -> NodeDbError { Error::InvalidLimitValue { clause, value } => { NodeDbError::invalid_limit_value(*clause, value.clone()) } - Error::RetryableSchemaChanged { descriptor } => { - NodeDbError::plan_error(format!("retryable schema change on {descriptor}")) - } + Error::RetryableSchemaChanged { .. } => NodeDbError::plan_error(e.to_string()), Error::RetryableLeaderChange { group_id, log_index, diff --git a/nodedb/tests/crash_harness/pgwire.rs b/nodedb/tests/crash_harness/pgwire.rs index bea03ad1a..8f9f64f8a 100644 --- a/nodedb/tests/crash_harness/pgwire.rs +++ b/nodedb/tests/crash_harness/pgwire.rs @@ -11,7 +11,8 @@ use std::time::{Duration, Instant}; use super::CrashHarness; /// Bounded retry budget for the `Error::RetryableSchemaChanged` condition -/// (rendered over pgwire as `XX000: retryable schema change on `). +/// (rendered over pgwire as `XX000: schema changed during execution +/// (); please retry`). /// /// The server already retries this condition server-side for ~750ms /// (`retry_on_schema_change`, `control/server/shared/retry.rs`, 5 attempts, @@ -30,15 +31,10 @@ const SCHEMA_CHANGE_RETRY_ATTEMPTS: usize = 5; const SCHEMA_CHANGE_RETRY_BACKOFF: Duration = Duration::from_millis(150); /// Substring of `Error::RetryableSchemaChanged`'s Display text -/// (`#[error("retryable schema change on {descriptor}")]` in -/// `nodedb/src/error.rs`). No distinct SQLSTATE is assigned to this -/// condition — `error_to_sqlstate` in `control/server/pgwire/types/error_map.rs` -/// has no arm for it, so it falls through to the generic -/// `sqlstate::INTERNAL_ERROR` (`XX000`) bucket shared by every other -/// unmapped error. Matching on that code alone would blanket-retry -/// unrelated internal errors, so the message text — which is the error -/// type's own stable Display string, not free-form prose — is the only -/// durable signal available. +/// (`#[error("schema changed during execution ({descriptor}); please retry")]` +/// in `nodedb/src/error/types.rs`). The message is the durable signal: a code +/// alone would blanket-retry unrelated internal errors, because the class this +/// condition carries depends on the mapper in front of it. /// The server was still reporting `RetryableSchemaChanged` when the /// client-side retry budget ran out. /// @@ -51,7 +47,7 @@ pub struct RetryableSchemaChange; fn is_retryable_schema_change(e: &tokio_postgres::Error) -> bool { e.as_db_error() - .is_some_and(|db| db.message().contains("retryable schema change")) + .is_some_and(|db| db.message().contains("schema changed during execution")) } /// Substring of the `calvin-submit` internal error text