Skip to content

Commit 2f8f4f1

Browse files
committed
fix(pgwire): answer a real class for the arm-less error variants
The blanket INTERNAL_ERROR fallback in error_to_sqlstate flattened 65 of the 101 crate::Error variants, so a statement reading a balance, hitting a period lock, a legal hold, a retention floor, a type guard, or a mirror write got XX000 while the crate's classify() map already carried the class. The fallback now borrows it through classify() and answers from the same numeric table the remote path reads. The bridge exposed one arm doing harm: RetryableSchemaChanged classified as a plan error, which reads as 'fix your SQL' for a condition the lease paths retry. It now classifies as a write conflict (40001), matching its contract.
1 parent 70ab060 commit 2f8f4f1

2 files changed

Lines changed: 113 additions & 2 deletions

File tree

‎nodedb/src/control/server/pgwire/types/error_map.rs‎

Lines changed: 109 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,14 @@ pub fn error_to_sqlstate(err: &crate::Error) -> (&'static str, &'static str, Str
225225
numeric_code_to_sqlstate(e.code()),
226226
e.message().to_string(),
227227
),
228-
_ => ("ERROR", sqlstate::INTERNAL_ERROR, err.to_string()),
228+
// A variant with no arm of its own carries its class on the error:
229+
// `classify` borrows it (the crate's one Error-to-NodeDbError map),
230+
// then this table answers instead of a blanket `XX000`.
231+
_ => (
232+
"ERROR",
233+
numeric_code_to_sqlstate(crate::error_classify::classify(err).code()),
234+
err.to_string(),
235+
),
229236
}
230237
}
231238

@@ -490,4 +497,105 @@ mod tests {
490497

491498
assert!(message.contains("timestamp"));
492499
}
500+
501+
/// A variant with no arm of its own still carries a class: the fallback
502+
/// borrows it through `classify` and this table answers it.
503+
#[test]
504+
fn arm_less_variants_keep_their_class_through_the_fallback() {
505+
use crate::Error;
506+
507+
let cases = vec![
508+
(
509+
Error::AppendOnlyViolation {
510+
collection: "c".into(),
511+
detail: "d".into(),
512+
},
513+
sqlstate::APPEND_ONLY_VIOLATION,
514+
),
515+
(
516+
Error::BalanceViolation {
517+
collection: "c".into(),
518+
detail: "d".into(),
519+
},
520+
sqlstate::BALANCE_VIOLATION,
521+
),
522+
(
523+
Error::InsufficientBalance {
524+
collection: "c".into(),
525+
key: "k".into(),
526+
detail: "d".into(),
527+
},
528+
sqlstate::CHECK_VIOLATION,
529+
),
530+
(
531+
Error::PeriodLocked {
532+
collection: "c".into(),
533+
detail: "d".into(),
534+
},
535+
sqlstate::PERIOD_LOCKED,
536+
),
537+
(
538+
Error::RetentionViolation {
539+
collection: "c".into(),
540+
detail: "d".into(),
541+
},
542+
sqlstate::RETENTION_VIOLATION,
543+
),
544+
(
545+
Error::LegalHoldActive {
546+
collection: "c".into(),
547+
detail: "d".into(),
548+
},
549+
sqlstate::LEGAL_HOLD_ACTIVE,
550+
),
551+
(
552+
Error::StateTransitionViolation {
553+
collection: "c".into(),
554+
detail: "d".into(),
555+
},
556+
sqlstate::STATE_TRANSITION_VIOLATION,
557+
),
558+
(
559+
Error::TransitionCheckViolation {
560+
collection: "c".into(),
561+
detail: "d".into(),
562+
},
563+
sqlstate::TRANSITION_CHECK_VIOLATION,
564+
),
565+
(
566+
Error::TypeGuardViolation {
567+
collection: "c".into(),
568+
detail: "d".into(),
569+
},
570+
sqlstate::TYPE_GUARD_VIOLATION,
571+
),
572+
(
573+
Error::TypeMismatch {
574+
collection: "c".into(),
575+
key: "k".into(),
576+
detail: "d".into(),
577+
},
578+
sqlstate::DATATYPE_MISMATCH,
579+
),
580+
(
581+
Error::MirrorReadOnly {
582+
database: "db".into(),
583+
},
584+
sqlstate::READ_ONLY_SQL_TRANSACTION,
585+
),
586+
(
587+
// Retryable in the lease paths, so the fallback must not read
588+
// as a plan/syntax error now that it answers through `classify`.
589+
Error::RetryableSchemaChanged {
590+
descriptor: "users".into(),
591+
},
592+
sqlstate::SERIALIZATION_FAILURE,
593+
),
594+
];
595+
596+
for (err, expected) in cases {
597+
let (_severity, state, _message) = error_to_sqlstate(&err);
598+
assert_eq!(state, expected, "{err:?}");
599+
}
600+
}
493601
}

‎nodedb/src/error_classify.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,8 +177,11 @@ pub(crate) fn classify(e: &Error) -> NodeDbError {
177177
Error::InvalidLimitValue { clause, value } => {
178178
NodeDbError::invalid_limit_value(*clause, value.clone())
179179
}
180+
// Retryable, like a write conflict: the descriptor-lease paths already
181+
// retry this variant, so it must not reach a client as a plan/syntax
182+
// error, which reads as "fix your SQL".
180183
Error::RetryableSchemaChanged { descriptor } => {
181-
NodeDbError::plan_error(format!("retryable schema change on {descriptor}"))
184+
NodeDbError::write_conflict("catalog", descriptor.clone())
182185
}
183186
Error::RetryableLeaderChange {
184187
group_id,

0 commit comments

Comments
 (0)