Skip to content

fix(pgwire): complete the shaper error classification and keep internal detail in the log - #359

Closed
EnRaiha wants to merge 3 commits into
mainfrom
fix/shaper-mapper
Closed

EnRaiha wants to merge 3 commits into
mainfrom
fix/shaper-mapper

Conversation

@EnRaiha

@EnRaiha EnRaiha commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

Why

numeric_code_to_sqlstate answered 22 of 81 codes and fell back to XX000 for the rest, so a quota, clone, move-tenant, mirror, sync, storage/WAL, cluster, config, or encryption fault reached every routed surface as an internal fault — including the shaper's own SERIALIZATION code, the exact "payload does not fit the projection" case the class should name.

An XX000-class failure also rendered its message verbatim, so a manifest path or invariant text could travel to a client on an error frame.

Fixes #338. Bottom of a two-PR stack.

What

  • Mapper completion (numeric_code_to_sqlstate, 22 → 81): the remaining 59 codes carry issue Routed response-shaper errors flatten to XX000; the numeric SQLSTATE mapper covers 20 of 81 codes #338's decided class — the constraint family mirrors the Data Plane table (236xx); clone, quota, move-tenant, mirror, sync, storage/WAL, cluster, config, and encryption follow the classes the local crate::Error table already names. INTERNAL, BRIDGE, and DISPATCH keep XX000 as explicit arms.

  • Six new sqlstate.rs constants: 22000, 22P02, 25006, 2BP01, 42P07, XX001.

  • Hygiene: new shaping_error_message; an XX000-class failure logs the detail with its code and sends one stable summary, every other class passes through.

  • Fallback bridge: error_to_sqlstate no longer ends in a blanket INTERNAL_ERROR. A variant
    with no arm of its own borrows its class through classify() and answers from this table, so the
    65 arm-less variants (balance, period lock, legal hold, retention, transitions, type guard, type
    mismatch, mirror, session and quota rejections, ...) stop reporting XX000. The bridge exposed
    RetryableSchemaChanged classifying as a plan error; it now classifies as a write conflict
    (40001), matching the retry contract its lease callers already rely on.

Validation

Run at the head of the stack (this PR plus the surface PR above it):

  • cargo check -p nodedb --all-targets — clean.
  • cargo test -p nodedb --lib error_map — 35 passed (mapper table covering all 59 completion codes, the three internal arms, hygiene replace/pass-through).
  • cargo test -p nodedb-types — 750 passed (sqlstate constants incl. the six new ones).
  • cargo clippy -p nodedb -p nodedb-types --all-targets -- -D warnings — clean.
  • cargo fmt --all -- --check — clean.

Notes

The streamed responder and the DDL dispatch render through this mapper in the PR stacked above.

Tradeoffs

Tradeoff: the bridge moves RetryableSchemaChanged from a plan error to a write conflict (40001). The class change is deliberate: the lease paths retry this variant by contract.

Copilot AI lite review requested due to automatic review settings September 20, 2026 11:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@EnRaiha EnRaiha added the run-ci Opt this PR into the full test suite; re-add to force a re-run label Sep 20, 2026
@EnRaiha
EnRaiha added this pull request to stack #363 September 20, 2026 14:54
…the log

The mapper answered 22 of 81 codes and fell back to XX000 for the rest, so a
quota, clone, move-tenant, mirror, sync, storage/WAL, cluster, config,
encryption, or shaper-raised code reached every routed surface as an internal
fault. The remaining 59 codes now carry the decided class, with six new
sqlstate.rs constants (22000, 22P02, 25006, 2BP01, 42P07, XX001); INTERNAL,
BRIDGE and DISPATCH keep XX000 as explicit arms.

shape_error_to_pg now renders its message through shaping_error_message: an
XX000-class failure logs the detail and sends one stable summary, so a
manifest path or invariant text cannot travel to a client on an error frame.

Tests: completion table (59 codes), internal arms, hygiene pass-through/replace.
types/mod.rs re-exported numeric_code_to_sqlstate, but the routed gateway
calls it by its full path and internal_message uses the module directly, so
the re-export was dead and Lint & Check failed on the unused import.
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.
@EnRaiha
EnRaiha removed this pull request from stack #363 September 21, 2026 02:37
@EnRaiha

EnRaiha commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main @ 4664c8213 and pushed (2f8f4f17c). The single conflict was in error_map.rs, where main's new Error::Shaping arm landed: that arm is kept and the fallback bridge follows it. Local evidence: cargo test -p nodedb --lib error_map — 37 pass. The branch is linear again (3 commits over main), so the streamed/DDL surfaces work can be resubmitted against main once this lands.

@EnRaiha
EnRaiha requested a review from farhan-syah September 21, 2026 05:25
@farhan-syah

Copy link
Copy Markdown
Member

Closing. Several classes in the completed table are wrong: SERIALIZATION → 22P02 reports internal encode failures as client input errors, STORAGE/WAL → 58030 renders raw detail (paths) to the client, and CLUSTER → 08006 tells drivers the connection dropped. The branch also conflicts with main after #360. The maintainers will settle the SQLSTATE table for #338 in-tree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-ci Opt this PR into the full test suite; re-add to force a re-run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Routed response-shaper errors flatten to XX000; the numeric SQLSTATE mapper covers 20 of 81 codes

3 participants