Repository navigation
fix: follow-up for 3.9.1 (startup_parameters) - #248
Merged
Merged
Conversation
added 10 commits
May 13, 2026 08:34
3.9.1 carries the codex-review follow-up for startup_parameters (correctness, semantics, observability, performance). Raising BDD fan-out from 4 to 6 cuts wall time on the 22-suite matrix without measurable flake increase on the timing-sensitive sleep-heavy lifecycle/SCRAM-reconnect suites.
H1: client startup filters operator-managed GUC names before merging the client StartupMessage so ParameterStatus matches what PG will actually use for the session. H2: auth_query MD5 retry swaps the refetched cache entry into the local snapshot. Without it, a rotated password matched on retry still built backend_auth and the per-user startup_parameters overlay from the stale row. H3: backend spawn fails with ServerStartupParameterRejection (54000) when the operator/auth_query cascade does not fit the StartupMessage budget, instead of silently shipping an empty packet or dropping the auth_query overlay. Config validation refuses the deterministic general+pool overflow at load time.
M1: case-insensitive canonicalization of tracked GUC names so a client `TIMEZONE` cannot slip past operator_managed_startup_keys and overwrite the operator default. M2: dynamic pool fast path now compares the incoming overlay hash against the live pool's frozen hash. On drift the pool is dropped and rebuilt against the current snapshot; without this a concurrent login could inherit a stale per-user startup_parameters set after an auth_query row update. M3: the auth_query startup_parameters reader accepts a native `json`/`jsonb` column without requiring a `::text` cast. The fallback uses `serde_json::Value` (enabled via the `with-serde_json-1` feature on tokio-postgres) and falls back to the existing string decoder. M6: when the auth_query overlay is dropped because it would push the cascade over budget, every key from the overlay is reported as `dropped_due_to_budget` in the admin/API view. The previous code classified overlay keys that happened to exist in the baseline as `stale`, sending operators to chase a RELOAD race that was not the cause.
M4: startup_parameter values in /api/config and /api/pools are now visible only to Admin. SSO callers see the same masked view as anonymous so a broad sso_allowed_users list cannot leak tenant routing tags, audit identifiers, or accidental secrets through those endpoints. M5: the immutable-fields list now matches full flattened keys (general.host, general.port, web.host, web.port). The previous bare segments never matched the flattened key paths the UI receives, so every field was rendered as reloadable even when a restart was actually required.
…ransport H5: a new startup-parameters.yaml rule group ships critical alerts for PG-side rejection of configured startup_parameters and for cascade overflow (both fail every backend spawn), plus warning alerts for malformed auth_query columns and the dedicated-mode drop signal. M11: web-sso.yaml carries a new PgDoormanWebSsoInsecureTransport warning so a regression in trusted_proxies / X-Forwarded-Proto plumbing under sso_require_https surfaces as its own alert instead of hiding inside the generic rejected-auth signal.
P1: ServerPool now precomputes the wire-ready merge of base_startup_parameters + per_user_startup_overlay once at pool creation, stores it as an Arc<BTreeMap<String, String>>, and hands out a borrow on every spawn/admin lookup. Static pools share the existing base Arc; dynamic pools clone the BTreeMap once instead of on every backend create. P2: Server::startup takes the Arc<HashSet<String>> of operator- managed key names directly from the pool instead of rebuilding the canonicalized set per spawn. The backend struct now stores the same Arc the pool/client paths read. P4: cache the BudgetDecision next to the resolved map so the spawn path skips the validation walk on every checkout. The single warn line that announces over-budget cascades moves to pool construction; per-spawn paths keep the metric increment and return the PG-style 54000 error from H3.
M12: reserved keys list now mentions role and session_authorization in both languages so an operator does not waste time on a config rejection without a documentation clue. M13: rewording of the budget-overflow paragraph spells out that general+pool alone can overflow once merged, that an auth_query overlay can push a previously fitting cascade over the cap, and that overflow now surfaces as SQLSTATE 54000 on the client side instead of a silent strip-down. M14: Russian observability section gains the pg_doorman_startup_parameters_dropped_total bullet so operators reading only RU docs see the pre-wire counter. M15: trusted_proxies entry warns that an empty list with sso_require_https=true rejects every SSO request behind a TLS terminator, and points at the CIDR to add. L2: Russian SQLSTATE wording drops the imaginary "startup family" category and names the real codes plus a clear "any other code PG returned for the StartupMessage rejection". L3: AuthGate login copy points at the active config file instead of hard-coding pg_doorman.toml; YAML and include-file deployments now read truthfully. L4: the AuthGate panel comment describes what the component actually does instead of pitching a visual style. L5: Russian auth_query reference clarifies that the startup_parameters column may be text, json, or jsonb and that the content must be a JSON object of per-user parameters. L6: troubleshooting phrase about the client-side username is rewritten into idiomatic Russian.
Adds focused test coverage for the most user-visible codex review fixes without bloating the BDD matrix: - M1: unit tests in src/server/parameters.rs lock in the case- insensitive canonicalisation of TimeZone and DateStyle so a future rewrite cannot silently drop the mixed-case mapping. - M3: a new auth_query_jsonb_fixture and BDD scenario in startup-parameters.feature show that an auth_query SELECT returning a native jsonb column applies the per-user GUC without a ::text cast. - M5: web-ui.feature exercises /api/config and checks that the bind-address fields are flagged restart-required so the SPA pill renders correctly. - H3: a Config::validate unit test pins the merged general+pool overflow refusal at config load.
Lists the protocol-correctness, auth_query, Web UI, observability, performance, and documentation changes that close the codex review gaps against 3.9.0. The configuration shape is unchanged; operators get a PG-style 54000 instead of a silent startup_parameters drop, admins keep startup_parameter visibility while SSO loses it, and the dynamic-pool fast path no longer inherits a stale per-user overlay.
Keep the user-visible behaviour, drop the per-codex-item breakdown (H1/M2/P4 callouts belonged in the PR description, not the release notes).
vadv
changed the title
added 8 commits
May 13, 2026 10:06
GH-H1: ServerStats was registered before the budget preflight ran, so an over-budget cascade left the stats entry stuck in sv_login. Resolve startup_parameters first; only register stats once the preflight has accepted the spawn. GH-M1: the slow-path race re-check in create_dynamic_pool now compares the overlay hash too. Two concurrent logins after an auth_query row update were able to reuse a pool the losing thread built with a stale snapshot until the next TTL or RELOAD. GH-M4: build_resolved_startup canonicalises every GUC name during the cascade merge. PostgreSQL GUC names are case-insensitive, so a pool override of `TimeZone` now wins over a general `timezone` instead of shipping both rows in StartupMessage and letting backend merge order pick the survivor. The baseline-only fallback used by OverlayDroppedBaselineKept and the admin/API read model both see the same canonical view.
GH-M2: the config-load gate now uses packet_and_body_bytes per user so a configuration whose body fits the operator budget but whose user/database/application_name push the full StartupMessage over PG cap fails pg_doorman -t instead of every backend spawn at runtime. GH-M3: SHOW STARTUP_PARAMETERS and /api/pools report every configured key as dropped_due_to_budget while the cascade is spawn-blocking. Previously the baseline keys still surfaced as applied even though no StartupMessage was going on the wire after the H3 fail-close. DevOps review: /api/config flags general.worker_threads, general.unix_socket_dir, general.backlog as restart-required so the SPA pill matches the runtime (these shape the tokio runtime and listener socket at process start and a SIGHUP cannot rebuild them). DevOps review: SHOW CONFIG admin command drops connect_timeout from its immutables list — connect_timeout is reloadable on SIGHUP and used to falsely paint as restart-required.
…roup C) GH-M5: when the dynamic-pool spawn fails with ServerStartupParameterRejection in passthrough mode, drop the dynamic pool and invalidate that user's cache entry. Without this the bad overlay sticks until cache_ttl expires, so every reconnect keeps hitting the same rejection even after the operator fixes the auth_query row. GH-M8: dispatch the auth_query startup_parameters column by PostgreSQL type. text/varchar continues through the original string decoder; json/jsonb goes straight through `serde_json::Value` into a shared validator. The previous fallback chain decoded text, failed, decoded as Value, re-serialised with `Value::to_string()`, and re-parsed — the second parse is gone. DBA review: when the MD5 retry refetches a verifier that is no longer MD5 (operator switched `password_encryption` mid-flight to SCRAM), invalidate the cache so the next reconnect hits `cache.get_or_fetch` and takes the SCRAM branch immediately instead of waiting for `cache_ttl`.
GH-M6: PgDoormanStartupParameterOverlayOversize used to match both `auth_query_overlay_oversize` (post-H3, every backend spawn for that user is rejected) and `auth_query_oversize` (parse-time, the overlay is dropped but the user still authenticates against general/pool defaults). The two reasons now have separate alerts and runbooks so an on-call does not get the spawn-blocking text for the degraded case. GH-M7: PgDoormanStartupParameterPgRejection drops from critical to warning. The metric only carries pool/sqlstate; a single bad auth_query row for one user can tick the counter without blocking the rest of the pool, so paging the rotation on a single increment was over-broad. The runbook now reads "at least one backend startup failed". DBA review: the fail-close SQLSTATE switches from 54000 (PG's own program_limit_exceeded) to 53400 (configuration_limit_exceeded). The rejection happens inside pg_doorman before any StartupMessage reaches PG, so the code that belongs to PG-internal limits was misleading both in client error messages and in log-search greps. DBA review: the EN tutorial now matches the code — text, json, and jsonb columns are all accepted without a `::text` cast.
Wraps the post-review tail. The PR is otherwise complete; these items came up across the second-pass reviews and were small enough to land in one commit. - BDD matrix comment in .github/workflows/bdd-tests.yml now documents the steady-state 6-parallel cap and why it does not reintroduce the timing-sensitive flakes a 4-cap originally guarded against. - The RU startup_parameters tutorial gains the same text/json/jsonb dispatch wording the EN tutorial already carries — the two were briefly out of sync. - Both tutorials note that operator-side edits to per-user startup_parameters apply only to new backend connections; the next client reconnect picks up the row and rebuilds the dynamic pool against the new snapshot. - Client startup no longer materialises a temporary HashMap when filtering out operator-managed GUC names; the filter loops directly into ServerParameters::set_param. A connect storm with configured startup_parameters saves one allocation per client. - CacheEntry now carries a precomputed per-user overlay hash, set in lockstep with the map via a single set_startup_parameters helper. The dynamic-pool fast path consumes the hash through a new parameter on create_dynamic_pool instead of re-running the sort + SipHash on every login. - Second-pass DBA review (MED): invalidate the auth_query cache before dropping the dynamic pool on ServerStartupParameterRejection so a racing reconnect cannot rebuild the same pool against the still-cached bad overlay.
…ents Rewrites the prose touched by the codex follow-up to neutral technical voice. The behavior in this commit is unchanged. - Tutorial pages and the changelog drop AI-marker phrasing and trim the breadth claims to what the code actually does. - web-sso.yaml runbooks read like the surrounding auth-query rules and avoid the "every new backend for this pool failed" superlative the metric labels cannot prove. - BDD scenarios and the new auth_query_jsonb_fixture lose the codex-MED breadcrumbs in comments; the cited file:line callouts belonged in the PR description, not the test. - Source comments in web routes, the client startup filter, and the route DTOs lose the change-marker prose for the same reason.
…ade and full-packet preflight - Cascade merge canonicalises GUC names per layer instead of after the raw-key merge. A pool `TimeZone` now correctly wins over a general `timezone`; previously both keys survived the exact-string merge and the canonical collision was resolved by `BTreeMap` iteration order, which let the less specific layer win. Implemented as `startup_parameters::cascade_canonical_keys`, used by the static, dedicated auth_query, and dynamic pool base builds plus the runtime resolver. `auth_query` overlay parse canonicalises keys before insertion. `canonicalize_param_name` now covers every `TRACKED_PARAMETERS` alias instead of only `TimeZone`/`DateStyle`. - Config-load full StartupMessage preflight now mirrors runtime inputs: `application_name` defaults to `"pg_doorman"` (the runtime default) instead of the empty string, and the dedicated auth_query `server_user` is validated alongside the static users. The split helper closes the gap where `pg_doorman -t` accepted a config that later failed every backend startup with 53400. - The auth_query json/jsonb decoder applies the same `MAX_OPERATOR_BUDGET` size gate the text decoder enforces. A large jsonb row is now dropped as `auth_query_oversize` instead of being cached and rejected during backend startup. - Fallback path returns `ServerStartupParameterRejection` unchanged. The preflight is host-independent, so the previous whitelist-clear / discovery retry served only to wrap the PG-style 53400 in a generic `ConnectError` and wipe a healthy whitelist on a configuration bug. - `PgDoormanStartupParameterDedicatedDrops` widens to a 1h increase window with `for: 5m`. The default positive `cache_ttl` is 1h, so the counter only ticks once per user per hour; the previous 15m window could miss a steady misconfiguration even with `for: 15m`. - The BDD matrix comment is rewritten alongside the other editorial cleanups in this PR.
…p I) - Cascade is now fully case-insensitive across the parameter surface, not just for the five tracked ParameterStatus names. The canonical key function returns the fixed spelling for tracked GUCs and ASCII-lowercase for everything else, so a general `work_mem` baseline and a pool `Work_Mem` override collapse to one cascaded entry instead of shipping both rows. The same canonical form drives operator_managed key set, auth_query overlay, dynamic pool drift hash, admin/Web read model, and config validation. IntervalStyle joins the tracked set with the spelling PG reports back at runtime. - Config validate now rejects same-layer canonical duplicates with a message pointing at both raw keys, instead of letting BTreeMap iteration order silently pick one. - FailureSummary keeps the first ServerStartupParameterRejection separately so a mixed-failure fallback round (one PG candidate rejecting the cascade plus transport failures on the rest) still surfaces the real SQLSTATE to the client instead of the generic ConnectError wrapper. - The auth_query JSON/JSONB path enforces MAX_OPERATOR_BUDGET on raw wire bytes through a custom FromSql wrapper before serde_json walks the value tree. The text path already had this pre-parse gate; the asymmetry that let a large jsonb row be fully decoded before rejection is closed. - Client startup reads the operator-managed key set through a focused lookup that returns only the Arc<HashSet<String>> the filter needs, instead of cloning the whole ConnectionPool on every authenticated connect. - CacheEntry holds one StartupOverlay snapshot pairing the per-user map with its precomputed overlay hash. The two values now move together by construction; direct field writes that could drift the map and the hash are gone. - The BDD matrix runs the new @Web-UI suite. The /api/config restart-required scenario added in an earlier commit now executes in CI. - Generated auth_query reference docs accept text/json/jsonb and call out the custom-domain caveat, matching the tutorial and the decoder. - pool config-load validate uses a distinct display kind for the dedicated auth_query.server_user message so operators don't hunt the name in pool_config.users. - Grafana dashboard JSON is regenerated to match the panel-prose changes that landed in the editorial pass. - The 3.9.1 changelog lists upgrade notes for the SQLSTATE switch (54000 → 53400) and the severity downgrade of the PG-rejection alert.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up patch release that closes the protocol-correctness, observability, and operability gaps codex review surfaced against 3.9.0. Built on top of PR #246 (
feat/startup-parameters); merge after it. The configuration shape is unchanged.Protocol correctness (High)
StartupMessagesoParameterStatusmatches what PostgreSQL will actually use for the session. The backend'ssync_parametersalready dropped the same keys on checkout; the two views now agree.auth_queryMD5 retry swaps the refetched cache snapshot into the local view before the dynamic pool is built. Without this, a rotated password matched on retry still builtbackend_authand the per-userstartup_parametersoverlay from the stale row.ServerStartupParameterRejection(SQLSTATE 54000) when the operator/auth_query cascade does not fit the StartupMessage budget, instead of silently shipping an empty packet or dropping the overlay. Config validation rejects the deterministicgeneral + pooloverflow at load time.Semantics (Medium)
TimeZone/DateStylecanonicalisation is case-insensitive so a clientTIMEZONEcannot slip pastoperator_managed_startup_keys.create_dynamic_poolfast path compares the incoming overlay hash against the live pool's frozen hash and drops + rebuilds on drift.json/jsonbcolumn without requiring a::textcast (falls back toserde_json::Value)./api/configand/api/poolsreveal startup_parameter values only toAdmin. SSO is treated like anonymous so a broadsso_allowed_userslist cannot leak tenant/audit tags or accidental secrets.IMMUTABLESlists full flattened keys (general.host,general.port,web.host,web.port). Previous bare-segment matcher never compared positively against the keys the API actually emits.dropped_due_to_budgetinstead ofstalewhen the auth_query overlay was dropped because of the budget, even if the baseline carries the same key.Observability (High/Medium)
monitoring/prometheus-rules/startup-parameters.yamlships critical alerts for PG-side rejection and cascade overflow (both fail every backend spawn), warnings for malformed auth_query columns and the dedicated-mode drop signal.PgDoormanWebSsoInsecureTransportalert covers thesso_require_httpstransport-gate regression so a misconfiguredtrusted_proxiessurfaces as its own alert.Performance
ServerPoolprecomputes the wire-ready merged map and budget classification once at pool creation. Backend spawns hand out a borrow on the cachedArc<BTreeMap>instead of cloning the map and re-running the validation walk per checkout. The canonicalised set of operator-managed GUC names is built once per pool and shared with the backend startup path throughServer::startup. The single warn line announcing over-budget cascades moves to pool construction; per-spawn paths keep the metric increment and return the PG-style 54000 error from H3.StalevsApplieddistinction.Documentation and CI
roleandsession_authorization.pg_doorman_startup_parameters_dropped_totalbullet and lose the imaginary "startup family" SQLSTATE phrasing.trusted_proxiesinteracts withsso_require_https.pg_doorman.toml; works for YAML and include-file deployments.