Unblock postgres_instance_config PATCH (#163) by sdairs · Pull Request #215 · ClickHouse/clickhousectl

sdairs · 2026-05-23T15:49:46Z

Summary

Flip every PgConfig field (23) plus both PostgresInstanceConfig nested fields (pgConfig, pgBouncerConfig) to Option<T> with skip_serializing_if = "Option::is_none", so Client::postgres_instance_config_patch can express a real partial update. Pair the change with 25 OPTIONALITY_EXEMPTIONS entries in spec_coverage_test.rs; the staleness detector will tell us automatically when upstream fixes the spec.
Re-enable the deferred PATCH round-trip in integration_postgres_test.rs (GET baseline → PATCH max_connections → poll-until-applied → PATCH back).
Add a gated CLICKHOUSE_CLOUD_POSTGRES_CONFIG_PROBE=1 probe phase that fires the 6 × 2 behaviour-matrix scenarios from the issue's follow-up comment via raw reqwest and prints a markdown table on stderr. Default off so it doesn't run on every integration test.

Closes #163.

Follow-ups

Capture the behaviour matrix and file the upstream spec issue (draft text already lives in Cover postgres_instance_config_post + postgres_instance_config_patch #163). To capture:
```
CLICKHOUSE_CLOUD_POSTGRES_CONFIG_PROBE=1 \
  cargo test -p clickhouse-cloud-api --test integration_postgres_test cloud_postgres_crud_lifecycle -- --ignored --nocapture
```
Once the spec is fixed upstream, the new OPTIONALITY_EXEMPTIONS entries become stale and field_optionality_matches_spec will flag them — remove the block.

Test plan

cargo build --tests (whole workspace)
cargo clippy --tests
cargo test (all non-ignored tests pass)
cargo test -p clickhouse-cloud-api --test spec_coverage_test field_optionality_matches_live_spec -- --ignored — 25 new exemptions all register against the live spec, no stale entries
cargo test -p clickhouse-cloud-api --test integration_postgres_test -- --ignored --nocapture (live cloud creds required)
CLICKHOUSE_CLOUD_POSTGRES_CONFIG_PROBE=1 cargo test -p clickhouse-cloud-api --test integration_postgres_test -- --ignored --nocapture — capture behaviour matrix for upstream

🤖 Generated with Claude Code

sdairs · 2026-05-23T17:31:34Z

Blocked: response shape returns string-encoded numbers

Marking this PR as draft and blocked. The request-shape fix in this branch works (PATCH/POST now succeed against the live API per the captured matrix), but the response shape can't be deserialised into the spec-declared types.

What we see

For pgConfig fields the spec declares as integer / number, the live API returns the value wrapped in a JSON string. Example response from a successful PATCH (matrix body 2):

{"result":{"pgConfig":{"max_connections":"200"},"pgBouncerConfig":{},"message":"…"}}

max_connections is "200" (string), not 200 (number). Deserialising into Option<i64> fails with:

JSON error: invalid type: string "107", expected i64 at line 1 column 46

This affects every strictly-numeric pgConfig field — max_connections, effective_io_concurrency, max_worker_processes, max_parallel_workers, max_parallel_workers_per_gather, max_parallel_maintenance_workers, random_page_cost. The mixed-type fields (type: ["string","integer"] like work_mem, *_timeout, etc.) are already typed as serde_json::Value, so they're unaffected.

The same asymmetry applies in both directions: requests must send JSON numbers (string-encoded values are rejected), but responses come back string-encoded. So we can't just flip the field types — the model would then serialise wrong on the request side.

Why this PR is paused

A client-side workaround (custom deserialize_with that accepts both number and string) was considered and rejected — we don't want to mangle types to paper over an API inconsistency. This needs to be fixed on the API side: response numeric fields should match the spec's declared integer/number types.

Next steps

File / link upstream API issue for response-side type mismatch on pgConfig numeric fields
Unblock once the API returns numbers as numbers
Re-enable the gated integration round-trip (patch pgConfig.max_connections in integration_postgres_test.rs)

cursor

Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.

^{❌ 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 f1e661b. Configure here.}

cursor · 2026-07-04T22:35:15Z

+                    },
+                )
+                .await?;
+        }


Probe mutates before stale baseline

Medium Severity

When the CLICKHOUSE_CLOUD_POSTGRES_CONFIG_PROBE is enabled, run_pg_config_probe mutates the Postgres instance configuration before the PATCH round-trip test. This means the round-trip test operates with a stale baseline from an earlier GET, which can lead to incorrect verification and leaves the instance in an altered state for settings other than max_connections.

^{Reviewed by Cursor Bugbot for commit f1e661b. Configure here.}

cursor · 2026-07-04T22:35:15Z

+                .max_connections
+                .as_ref()
+                .and_then(pg_config_value_as_i64)
+                .unwrap_or(100);


Wrong default max_connections baseline

High Severity

The test's baseline_max_connections calculation defaults to 100 if the field is missing or unparseable. This can cause the test to PATCH and then reset the live instance's max_connections to an unintended value, altering its actual configuration.

^{Reviewed by Cursor Bugbot for commit f1e661b. Configure here.}

cursor · 2026-07-04T22:35:15Z

+        if env::var("CLICKHOUSE_CLOUD_POSTGRES_CONFIG_PROBE")
+            .ok()
+            .filter(|s| !s.is_empty())
+            .is_some()


Probe env accepts any value

Medium Severity

The CLICKHOUSE_CLOUD_POSTGRES_CONFIG_PROBE environment variable check enables the destructive behavior-matrix probe for any non-empty value, including "0" or "false." This deviates from standard boolean environment variable parsing and can unintentionally trigger twelve live POST/PATCH calls.

^{Reviewed by Cursor Bugbot for commit f1e661b. Configure here.}

sdairs · 2026-07-04T22:56:19Z

Follow-up: `run_pg_config_probe` looks like a one-off diagnostic

During code review, run_pg_config_probe (in tests/integration_postgres_test.rs) reads as a one-off evidence-gathering tool rather than a permanent test:

It's gated behind CLICKHOUSE_CLOUD_POSTGRES_CONFIG_PROBE so it never runs in normal CI.
It's StepKind::NonBlocking so it can't fail the test run.
Its output is a markdown table printed to stderr — not an assertion, not a regression guard. The table is meant to be pasted into the upstream spec issue (Cover postgres_instance_config_post + postgres_instance_config_patch #163).

Once the spec issue is resolved and the matrix is captured, this function has no ongoing purpose and becomes dead weight in the integration test binary. It also duplicates the library's auth/base-URL handling (raw reqwest::Client::new() + manual env-var reads instead of going through Client), which will silently drift if the library's env-var names or defaults change.

Suggested follow-up:

Drop it from this PR — run the probe as a local scratch file, paste the results into the spec issue, and keep only the permanent test coverage (the PATCH round-trip + serialization tests). Or,
If it should stay in-repo, move it to a standalone #[ignore] test or an examples/ binary so it's clearly separated from the integration suite, and document that it's for spec-issue evidence gathering only.

This is non-blocking for merging the PR — the PgConfig optionality fix itself is correct and well-tested — but worth resolving before the probe code calcifies.

The `pgConfig` schema has no `required` array and no field descriptions start with "Optional", so the resolver marks every property required and the generated model emits bare `T` fields. Serializing `PgConfig { max_connections: 200, ..Default::default() }` then sends zero/null/first-variant for every other property, and the live API rejects all of them with `Validation failed for following fields: pg_config.*`. The wrapping `postgresInstanceConfig` also lists both nested objects as required, making a partial PATCH unrepresentable. Flip all 23 `PgConfig` fields and both `PostgresInstanceConfig` nested fields to `Option<T>` with `skip_serializing_if = "Option::is_none"`, add matching `OPTIONALITY_EXEMPTIONS` entries (caught automatically as stale when the spec is fixed upstream), and re-enable the deferred PATCH round-trip in the postgres integration test. The integration test also gains a gated `CLICKHOUSE_CLOUD_POSTGRES_CONFIG_PROBE=1` phase that captures the 6×2 behaviour matrix the upstream report needs to cite real responses rather than assumptions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The behaviour matrix captured in #163 shows the live API rejects any postgresInstanceConfig PATCH or POST body that omits either nested object: omitting pgBouncerConfig yields `BAD_REQUEST: request body.pgBouncerConfig: 'undefined'`, omitting pgConfig yields the symmetric error. Sending `{}` for either is accepted (matrix body 2 → 200). The previous commit had flipped both to Option<T> with skip_serializing_if so partial PATCHes could send only the changed half, but that shape doesn't match the API. Flip pg_config and pg_bouncer_config on PostgresInstanceConfig back to required (bare T with #[serde(default)] so the default envelope still serialises to `{ "pgConfig": {}, "pgBouncerConfig": {} }`), drop the two corresponding OPTIONALITY_EXEMPTIONS entries, and update the fixture/integration tests to construct the new shape. Inner pgConfig fields stay Option<T> with skip_serializing_if — those remain opt-in per the spec's all-optional partial-update semantics. Response-side deserialisation is still broken (the API returns strictly-numeric pgConfig fields wrapped in JSON strings), which is tracked separately and blocks the integration round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The refreshed OpenAPI snapshot (#224) now types most pgConfig properties as string-or-number (["string","integer"], random_page_cost as ["string","number"]), matching the live API's behaviour of returning numeric values wrapped in JSON strings. Follow the existing PgConfig convention of serde_json::Value for multi-type fields and flip the remaining typed numerics — max_connections, effective_io_concurrency, max_parallel_maintenance_workers, max_parallel_workers, max_parallel_workers_per_gather, max_worker_processes (i64) and random_page_cost (f64) — to Option<serde_json::Value>. This unblocks the GET path: Option<i64> failed to deserialize the string-wrapped values the live endpoint returns. The enum-backed plain string fields (default_transaction_isolation, ssl_min_protocol_version, wal_compression) keep their enum types. Also re-add the "pgBouncerConfig": {} assertion to the POST/PATCH body_partial_json matchers in client_test.rs — always sending both nested objects is load-bearing (#163) — and make the integration round-trip tolerant of string-wrapped numbers via a shared extractor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

iskakaushik

LGTM

The base branch was changed.

sdairs requested a review from iskakaushik as a code owner May 23, 2026 15:49

sdairs had a problem deploying to cloud-integration May 23, 2026 15:49 — with GitHub Actions Failure

sdairs marked this pull request as draft May 23, 2026 17:31

sdairs added the blocked Can't be progressed label May 23, 2026

sdairs had a problem deploying to cloud-integration May 23, 2026 17:35 — with GitHub Actions Error

sdairs force-pushed the issue-163-pg-config-optionality branch from d7b6491 to f1e661b Compare July 4, 2026 22:30

sdairs had a problem deploying to cloud-integration July 4, 2026 22:30 — with GitHub Actions Failure

sdairs changed the base branch from main to issue-224-openapi-drift July 4, 2026 22:30

sdairs marked this pull request as ready for review July 4, 2026 22:32

cursor Bot reviewed Jul 4, 2026

View reviewed changes

sdairs removed the blocked Can't be progressed label Jul 4, 2026

sdairs had a problem deploying to cloud-integration July 5, 2026 13:14 — with GitHub Actions Failure

sdairs mentioned this pull request Jul 5, 2026

Flaky postgres integration test: read replica phase races the primary's first backup #282

Closed

sdairs and others added 3 commits July 5, 2026 16:05

sdairs force-pushed the issue-163-pg-config-optionality branch from f1e661b to 519c306 Compare July 5, 2026 15:06

sdairs had a problem deploying to cloud-integration July 5, 2026 15:06 — with GitHub Actions Failure

sdairs temporarily deployed to cloud-integration July 5, 2026 15:21 — with GitHub Actions Inactive

iskakaushik previously approved these changes Jul 6, 2026

View reviewed changes

Base automatically changed from issue-224-openapi-drift to main July 6, 2026 20:38

iskakaushik approved these changes Jul 6, 2026

View reviewed changes

sdairs merged commit 0d689cb into main Jul 6, 2026
4 of 5 checks passed

sdairs deleted the issue-163-pg-config-optionality branch July 6, 2026 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Unblock postgres_instance_config PATCH (#163)#215

Unblock postgres_instance_config PATCH (#163)#215
sdairs merged 3 commits into
mainfrom
issue-163-pg-config-optionality

sdairs commented May 23, 2026

Uh oh!

sdairs commented May 23, 2026

Uh oh!

cursor Bot left a comment

Uh oh!

cursor Bot Jul 4, 2026

Uh oh!

cursor Bot Jul 4, 2026

Uh oh!

cursor Bot Jul 4, 2026

Uh oh!

sdairs commented Jul 4, 2026

Uh oh!

iskakaushik left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

sdairs commented May 23, 2026

Summary

Follow-ups

Test plan

Uh oh!

sdairs commented May 23, 2026

Blocked: response shape returns string-encoded numbers

What we see

Why this PR is paused

Next steps

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot Jul 4, 2026

Choose a reason for hiding this comment

Probe mutates before stale baseline

Uh oh!

cursor Bot Jul 4, 2026

Choose a reason for hiding this comment

Wrong default max_connections baseline

Uh oh!

cursor Bot Jul 4, 2026

Choose a reason for hiding this comment

Probe env accepts any value

Uh oh!

sdairs commented Jul 4, 2026

Follow-up: run_pg_config_probe looks like a one-off diagnostic

Uh oh!

iskakaushik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Follow-up: `run_pg_config_probe` looks like a one-off diagnostic