wallet+db: handle SQL backend errors by yyforyongyu · Pull Request #1200 · btcsuite/btcwallet

yyforyongyu · 2026-03-30T11:17:34Z

Summary

introduce the shared SQL error framework in wallet/internal/db/err, including the shared taxonomy, wrapper, normalization helpers, stats collector, and package README
add backend-specific SQL error mapper packages in wallet/internal/db/postgres and wallet/internal/db/sqlite, with same-package tests and private helper details
add the shared wallet/internal/db/runtime helper framework for retry, unhealthy-store checks, transaction execution, and ambiguous commit handling
keep this PR focused on framework code only; production store wiring and runtime integration remain follow-up work once store code moves into the backend packages

Follow-up Plan

move store code into wallet/internal/db/postgres and wallet/internal/db/sqlite
integrate production normalization, stats recording, and runtime/store reaction logic in their final package locations
wire the shared runtime helper package on top of the backend-owned store implementations

Testing

make lint
GOWORK=off go test ./wallet/internal/db/... ./wallet
GOWORK=off go test ./wallet/internal/db/runtime
GOWORK=off go test -run '^$' -bench . ./wallet/internal/db/runtime

wallet/internal/db/dberrors.go

wallet/internal/db/dberrors_pg.go

wallet/internal/db/sqlite_dberrors.go

wallet/internal/db/postgres_dberrors.go

wallet/internal/db/sqlite_dberrors.go

wallet/internal/db/dbstats.go

yyforyongyu

LGTM ✅ (gpt-5.3-codex)

wallet/internal/db/postgres_dberrors.go

wallet/db_stats.go

yyforyongyu

LGTM 🛡️

Error classification is correct — verified all PG SQLSTATE codes and SQLite result codes against official docs. Classification ordering in classifySQLError is sound (context errors first, then backend-specific, then generic connection, then permanent fallback). Stats counters are atomic. Error.Is correctly matches both class and reason sentinels.

Two minor observations (not bugs):

Stats recorded during constructor error paths (PingContext/ApplyMigrations failure) are discarded since the store is never returned.
A few common PG constraint codes (23502, 23P01) are not explicitly mapped but fall through correctly to permanent/unknown.

(claude-opus-4-6)

yyforyongyu · 2026-03-30T20:10:54Z

I've addressed both observations in the latest commit:

Mapped 23502 (not_null_violation) and 23P01 (exclusion_violation) to ErrorClassPermanent and ErrorReasonConstraint.
Modified the DB constructors (NewPostgresStore and NewSqliteStore) to return the instantiated store alongside the initialization error during PingContext/ApplyMigrations failures, allowing callers to inspect store.DBStatsSnapshot(). Tested via db_connectors_test.go.

yyforyongyu

LGTM 🎲

The PR now includes:

Detailed docs for error sentinels explaining when they occur
Added and private sentinel types for type checking
Switched to a switch-based approach derived from
Clear documentation on pg and sqlite error specs
Backend resolution happens before classification in

Good work addressing all the feedback! ()

yyforyongyu · 2026-03-30T20:13:43Z

LGTM 🚀 (gemini-3.1-pro-preview)

yyforyongyu

LGTM 🎲

The PR now includes:

Detailed docs for error sentinels explaining when they occur
Added errorClassSentinel and errorReasonSentinel private sentinel types for type checking
Switched reasonRecorders to a switch-based approach derived from errorReasonSentinel
Clear documentation on pg and sqlite error specs
Backend resolution happens before classification in classifySQLError

Good work addressing all the feedback! (glm5)

wallet/internal/db/dberrors.go

wallet/internal/db/dbstats.go

wallet/internal/db/migrations.go

wallet/internal/db/sqlite.go

Add the shared SQL error taxonomy, wrapper, and normalization helpers in the new db/err package. The API now uses shorter shared Class and Reason names so call sites avoid the earlier stutter. This also moves the shared error tests into the same package and adds docs for the remaining internal helpers.

Add the PostgreSQL SQLSTATE mapper in the new db/postgres package. The mapper converts PostgreSQL driver and transport failures into the shared SQL error model while keeping backend-specific helper details private to the package. This also moves the backend-specific mapper tests into the same package.

Add the SQLite result-code mapper in the new db/sqlite package. The mapper converts SQLite driver errors into the shared SQL error model while keeping backend-specific helper details private. This also moves the backend-specific mapper tests into the same package.

Add the shared SQL error stats collector and snapshot types to the db/err package. This keeps classification-driven metrics with the shared error framework instead of wiring them through top-level db production code first. This also adds focused unit coverage for the shared stats behavior.

Document the purpose of the shared db/err package and the intended boundaries between shared error code, backend-specific mapping, and runtime/store reaction policy. This keeps the staged package split explicit while production integration stays in follow-up work.

Add the shared runtime helper package for SQL backends. The new helpers cover read retries, unhealthy-store checks, transaction execution, and ambiguous commit wrapping without wiring into backend store code yet. This keeps the runtime framework reviewable while store integration lands in follow-up work.

Add focused tests and small benchmarks for the shared runtime helper package. The coverage exercises retry, unhealthy-store, begin, and ambiguous-commit paths while the benchmarks cover the steady-state and transient-retry read flows.

Rename the shared error package declaration to err, keep dberr as\nthe import alias at call sites, and simplify the shared naming to\nClass and Reason throughout the package. The reason stringer now uses\none explicit switch and the same-package tests follow the renamed\npackage cleanly.\n\nThis also fills in the remaining mapper and helper docs so the shared\nerror framework matches the repo's Go documentation rules.

Rename the shared runtime hook interfaces and ambiguous-commit\nerror types to make their purpose clearer. Read retries now take\nexplicit caller-provided settings so future wallet or backend config\ncan own the retry policy instead of the shared helper package.\n\nThis also adds the missing helper docs and expands the tiny test\nhelpers to match the local multiline style.

yyforyongyu

LGTM 🎲

All review comments have been addressed. The PR now has:

Clean package structure with shared err package and backend-specific postgres/sqlite packages
Well-documented enums with Valid() methods for backend, class, and reason
Clear Reason.Class() method deriving policy from reason
Proper normalization helper that stays side-effect free
README documenting package ownership boundaries

Good work on the iterations! (glm5)

yyforyongyu · 2026-04-01T15:30:28Z

LGTM 🚀 (gemini-3.1-pro-preview)

yyforyongyu

LGTM ✅ (gpt-5.3-codex)

yyforyongyu

LGTM 🛡️

All 21 PG SQLSTATE codes and 13 SQLite result codes verified against official docs — correctly classified. Reason.Class() range partitioning is correct. Normalize() ordering is sound (context first, existing wrappers preserved, backend-specific before generic transport, permanent fallback). pgconn.PgError is matched before SafeToRetry/Timeout so SQLSTATE codes are never shadowed. Stats counters atomic throughout. Retry loop boundaries correct with proper overflow handling in retryDelay. All 13 review threads resolved.

(claude-opus-4-6)

yyforyongyu · 2026-04-02T10:58:20Z

LGTM 🛡️ (qwen3.5-397b-a17b)

All review threads resolved. The PR now includes:

Comprehensive error classification with detailed docs for each sentinel
Private sentinel types (errorClassSentinel, errorReasonSentinel) for type checking
Switch-based error classification with proper backend resolution
Complete SQLSTATE coverage for constraint violations (23502, 23503, 23504, 23505, 23514, 23P01)
Stats API exposing startup failures via DBStats()
Proper comment formatting throughout

Error classification logic is sound and well-documented.

yyforyongyu added this to the Introduce SQL store milestone Mar 30, 2026

yyforyongyu self-assigned this Mar 30, 2026

saubyk added this to lnd v0.22 Mar 30, 2026

github-project-automation bot moved this to Todo in lnd v0.22 Mar 30, 2026

saubyk moved this from Backlog to In progress in lnd v0.22 Mar 30, 2026

yyforyongyu force-pushed the handle-sql-error branch 2 times, most recently from d666be3 to 46801d8 Compare March 30, 2026 17:25

yyforyongyu changed the base branch from sql-wallet to adr-0006-tests March 30, 2026 19:21

yyforyongyu force-pushed the handle-sql-error branch from 46801d8 to 8c2cd43 Compare March 30, 2026 19:21

yyforyongyu commented Mar 30, 2026

View reviewed changes

wallet/internal/db/postgres_dberrors.go Outdated Show resolved Hide resolved

yyforyongyu commented Mar 30, 2026

View reviewed changes

wallet/db_stats.go Outdated Show resolved Hide resolved

yyforyongyu commented Mar 30, 2026

View reviewed changes

yyforyongyu force-pushed the handle-sql-error branch from 4756bf7 to bb6fdf3 Compare March 31, 2026 06:59

yyforyongyu commented Mar 31, 2026

View reviewed changes

yyforyongyu force-pushed the handle-sql-error branch from bd96423 to 77c893f Compare April 1, 2026 09:41

yyforyongyu added 5 commits April 1, 2026 19:51

yyforyongyu force-pushed the handle-sql-error branch from 77c893f to 4fa0531 Compare April 1, 2026 12:39

yyforyongyu force-pushed the adr-0006-tests branch from 0511555 to 91a6ee6 Compare April 1, 2026 12:46

yyforyongyu force-pushed the handle-sql-error branch from 4fa0531 to 215c824 Compare April 1, 2026 12:47

db/runtime: add helper tests and benchmarks

08832b6

Add focused tests and small benchmarks for the shared runtime helper package. The coverage exercises retry, unhealthy-store, begin, and ambiguous-commit paths while the benchmarks cover the steady-state and transient-retry read flows.

yyforyongyu force-pushed the handle-sql-error branch from 6a30fad to 08832b6 Compare April 1, 2026 13:25

yyforyongyu added 2 commits April 1, 2026 23:26

yyforyongyu commented Apr 1, 2026

View reviewed changes

Conversation

yyforyongyu commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Follow-up Plan

Testing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

yyforyongyu commented Mar 30, 2026

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

yyforyongyu commented Mar 30, 2026

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

yyforyongyu commented Apr 1, 2026

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

yyforyongyu commented Apr 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yyforyongyu commented Mar 30, 2026 •

edited

Loading