Skip to content

Conversation

@simvlad
Copy link
Contributor

@simvlad simvlad commented Jul 25, 2025

What changed?

Changes version compability checks to pass the real logger, so that the error messages would be seeing in the log

Why?

Before, the error was generic and there were no logs indicating the real error (see #7939):

sql schema version compatibility check failed: unable to read DB schema version keyspace/database: temporal error: no usable database connection found

With this change, the log would contain the actual error:

{"level":"error","ts":"2025-07-25T15:30:49.660-0700","msg":"sql handle: unable to refresh database connection pool","error":"pq: no pg_hba.conf entry for host \"127.0.0.1\", ...

The alternative solution would be to return the error, but the design of common/persistence/sql/sqlplugin/db_handle.go suggests we hold the abstraction boundaries, by returning generic errors and logging specific errors.

How did you test it?

  • built
  • run locally and tested manually
  • covered by existing tests

@simvlad simvlad requested a review from yycptt July 25, 2025 22:56
@simvlad simvlad requested a review from a team as a code owner July 25, 2025 22:56
Copy link
Member

@yycptt yycptt left a comment

Choose a reason for hiding this comment

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

the design of common/persistence/sql/sqlplugin/db_handle.go suggests we hold the abstraction boundaries, by returning generic errors and logging specific errors.

I was thinking only return the error on NewDatabaseHandle which eagerly sets up the connection. Didn't realize we are using no-op logger today and not even logging the real error. This approach LGTM.

@simvlad simvlad merged commit 4786548 into main Jul 28, 2025
85 of 87 checks passed
@simvlad simvlad deleted the simvlad/log-errors-version-compatibility branch July 28, 2025 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unclear error message on Postgres misconfiguration: 'no usable database connection found'

3 participants