Skip to content

Correct local Postgres diagnostics and help - #548

Open
sdairs wants to merge 2 commits into
stack/460-validate-local-postgresfrom
stack/465-postgres-diagnostics
Open

Correct local Postgres diagnostics and help#548
sdairs wants to merge 2 commits into
stack/460-validate-local-postgresfrom
stack/465-postgres-diagnostics

Conversation

@sdairs

@sdairs sdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • use Postgres-specific validation and runtime errors instead of ClickHouse execution errors
  • provide sanitized, platform-aware Docker constructor and ping guidance for Desktop/Engine, permissions, contexts, rootless Docker, and DOCKER_HOST
  • correct Postgres start help for managed labels and omitted versus explicit ports
  • restore actionable psql launch context and remove redundant Postgres error prefixes
  • add exact help, fake-socket, Docker classification, and missing-psql subprocess coverage

Tests

  • cargo fmt --all --check
  • cargo build -p clickhousectl
  • cargo check -p clickhousectl --all-targets
  • cargo test -p clickhousectl
  • cargo clippy -p clickhousectl --all-targets -- -D warnings
  • bash -n scripts/test-postgres-integration.sh
  • direct CLI verification of the integration-script port-zero and unsupported-version diagnostics

The full Docker-backed scripts/test-postgres-integration.sh was not run because the configured OrbStack socket at ~/.orbstack/run/docker.sock is unavailable.

Closes #465

Comment thread crates/clickhousectl/src/local/docker.rs
@sdairs

sdairs commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

Carry-over review from PR #509 (old PR for #465). This PR has genuinely better engineering than #509 (sanitized Docker failure causes with no-leak tests vs #509 echoing raw bollard text/paths; structured io::ErrorKind/source-chain classification; single Error::Postgres variant). But it carries over one directly violated sdairs review comment and several defects. Required:

  1. Remove the data-path help sentence sdairs explicitly rejected on Correct local Postgres diagnostics and help #509. Correct local Postgres diagnostics and help #509 had a pending inline review comment from sdairs on crates/clickhousectl/src/local/cli.rs (start help hunk, on the data-path line): "unnecessary - does not assist in usage of the cli", followed by his issue comment: "Addressed the pending feedback in 86dde3f: removed the non-actionable data-path/bind-mount sentence from local postgres start --help and updated its exact snapshot." In this PR that exact sentence is restored at crates/clickhousectl/src/local/cli.rs:~463 ("Data persists at .clickhouse/servers/-pg/data/ and is bind-mounted into the container.") — and worse, pinned in a test (tests/local_postgres_start_validation_test.rs:~228, postgres_start_help_matches_managed_runtime_behavior), so removing it later breaks the suite. Tension: issue Correct local Postgres diagnostics and help #465's acceptance criteria literally say "Help documents the versioned data path", so if the sentence is kept, state explicitly in the PR description that the earlier review position is being withdrawn; otherwise delete it from help and from the pinned snapshot.

  2. Restore psql diagnostic context. exec_host_psql (postgres.rs:~731) now returns Error::Postgres(err.to_string()), so a user with no psql installed sees Postgres error: No such file or directory (os error 2) with no mention of psql. Correct local Postgres diagnostics and help #509 wrapped it ("could not execute psql: ...") and had a subprocess test for the missing-psql case. Add the wrapper and the test.

  3. Fix the Windows socket/named-pipe contradiction (open Cursor Bugbot finding here). docker_failure_cause (docker.rs:~157-177) describes MissingSocket/PermissionDenied as socket failures on every platform while the Windows docker_guidance (docker.rs:~188) says named-pipe permissions — contradictory on Windows; and assert_platform_guidance in tests/local_docker_diagnostics_test.rs:~24-44 asserts "socket" on all platforms, so the test cannot catch it.

  4. Update scripts/test-postgres-integration.sh expectations. This PR changes the diagnostics this script greps (--port 0 wording via the stack base's clap range, not supported version wording) and leaves the edge-cases suite red. (Inherited from Validate local Postgres start options #547, but this is the diagnostics PR and should carry the script update.)

  5. Minor: message composition produces redundant "Postgres error: Postgres port 5432 is already in use" and "Postgres error: Postgres '' ... is gone" — pinned verbatim in tests; decide acceptable wording. HttpStatus/TimedOut classification branches have no test coverage. --port 0 handling in resolve_port is dead at the CLI layer (clap catches it first) — remove or keep for the API boundary.

@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch from 1f3b100 to bf62f15 Compare August 26, 2026 18:58
@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch 2 times, most recently from e7741da to a1b1a95 Compare August 26, 2026 19:03
@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch 2 times, most recently from bfb5980 to 27ebeca Compare August 26, 2026 19:10
@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch from 27ebeca to 058150c Compare August 26, 2026 19:12
@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch 2 times, most recently from 0bf7a6a to c42857c Compare August 26, 2026 19:24
@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch from c42857c to 239de49 Compare August 26, 2026 19:28
@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch 2 times, most recently from 9571807 to ba6d568 Compare August 26, 2026 19:34
@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch from ba6d568 to 70107a9 Compare August 26, 2026 19:36
@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch 3 times, most recently from f9fbdd7 to 469ae2f Compare August 26, 2026 20:35
@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch from 469ae2f to 1ededb8 Compare August 26, 2026 20:38
@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch from 1ededb8 to 46f544b Compare August 26, 2026 20:41
@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch from 46f544b to 83b41fc Compare August 26, 2026 20:44
@sdairs
sdairs force-pushed the stack/465-postgres-diagnostics branch from 83b41fc to a47bdda Compare August 26, 2026 20:49

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 621ab59. Configure here.

} else if chain.contains("connection refused") {
DockerFailureKind::ConnectionRefused
} else if chain.contains("no such file") || chain.contains("not found") {
DockerFailureKind::MissingSocket

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Docker errors misclassified as missing socket

Low Severity

The Docker connect fallback treats any error-chain text containing not found as MissingSocket. A TCP DOCKER_HOST with a bad hostname typically surfaces glibc's Name or service not found, so ping failures are reported as a missing Docker socket and the platform guidance then talks about sockets instead of the host lookup.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 621ab59. Configure here.

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.

Correct local Postgres diagnostics and help

1 participant