Repository navigation
feat: per-pool startup_parameters (GUC injection) - #246
Merged
Merged
Conversation
added 9 commits
May 11, 2026 20:15
…ation Adds operator-supplied PostgreSQL GUC injection at config level (general and per-pool). Field is BTreeMap<String, String>, validated for reserved keys (user/database/replication/options/_pq_.*), syntax (PG GUC naming including namespaced like auto_explain.log_min_duration), null-bytes in values, and overall 15 KiB operator budget within PG's 16 KiB StartupMessage cap. No runtime behavior change yet: nothing reads the field. Wire-format integration and resolution land in subsequent commits on this branch.
The previous commit chose a 15 KiB operator budget anchored on an incorrect 16 KiB ceiling. PostgreSQL's actual cap is 10 000 bytes (MAX_STARTUP_PACKET_LENGTH, src/include/libpq/pqcomm.h), enforced as an anti-DoS measure on every connection. A config that previously passed validation with parameters totalling 9.5-15 KiB would have been accepted by pg_doorman and silently rejected on the wire at every backend startup; the budget now reflects the real ceiling. Also pulls back operator-visible doc strings in the auto-generated reference config and on the General/Pool fields. The earlier text promised that parameters become 'pg_settings.reset_val' and cascade through auth_query - both true of the eventual feature but neither shipped by this commit. The docs now describe what is in fact delivered today: validation at config load.
Extends the StartupMessage builder to carry an additional set of operator-supplied key/value pairs. Keys are serialized in iteration order after the required user/application_name/database triple, with one trailing NUL terminator. When the supplied map contains application_name, that value wins over the pg_doorman-managed default (D5/B2 of the design - operator-overrideable). Wire-format change only: every current caller passes an empty map, so on-wire output is byte-identical to the prior format. Subsequent commits on this branch wire the resolver and per-pool quarantine in.
Two leaf modules with no integration yet: * src/pool/startup_resolver.rs: pure function that merges the general/pool/auth_query maps per the D1 cascade (union by key, more specific wins, auth_query > pool > general). * src/server/quarantine.rs: per-pool consecutive-rejection counter with TTL release. record_rejection returns Counting/JustQuarantined/ AlreadyQuarantined so callers can drive logs and metrics; release is TTL-only - keys that get quarantined are skipped in the StartupMessage, so a later success says nothing about whether the operator-supplied value was fixed. Adds two general-level knobs: startup_parameter_quarantine_threshold (default 3) and startup_parameter_quarantine_ttl (default 300000 ms).
Earlier commits left shorthand references like D5/B2, D7, D8 in code comments and rustdoc. Those identifiers point to a brainstorm document that is intentionally gitignored, so any reader outside the original session has no way to resolve them. Comments now describe the rule in prose: operator-supplied value wins over the pg_doorman-managed default for application_name; auth_query parameters are dropped in dedicated mode because one shared backend serves multiple users; quarantine releases by TTL only because a key that has been skipped offers no evidence of being fixed. No behavior change; comment-only.
* messages::protocol::startup() (already taking the operator map since the previous commit on this branch) is now called with the cascade- resolved map. Before sending, currently-quarantined keys are stripped via QuarantineState::filter_active_keys. * On PG ErrorResponse with sqlstate 22023 / 42704 / 42501 and a non- empty operator map, pg_doorman extracts the failing parameter name via the literal pattern `parameter "..."` (hand-rolled to keep regex in dev-deps) and records the rejection in the pool-scoped QuarantineState. After N consecutive rejections of the same key, subsequent backend spawns drop that key from the StartupMessage for a configurable TTL. * extract_parameter_name() pure helper added to startup_error.rs with unit tests covering unknown / invalid-value / insufficient-privilege / namespaced and no-match cases. Call sites currently pass an empty map and a per-call QuarantineState constructed from the general-level knobs; the pool-owned shared Arc ships in the next commit. The now-unused handle_startup_error stays in the tree behind #[allow(dead_code)] as a reference for the pre- quarantine behavior; cleanup is left to a follow-up.
The auth_query SQL contract gains an optional text column 'startup_parameters' holding a JSON object: its string-typed entries become the per-user GUC pg_doorman injects when it opens a backend on the user's behalf. SQL that does not return the column continues to work unchanged. Operator config validation rules (reserved keys, GUC name syntax, no null bytes) are reused: invalid entries are dropped with a warning, unparseable JSON yields an empty set with a warning, but auth never fails because of an operator config error in the query result. In dedicated mode (auth_query.server_user set, one shared backend pool under that identity) per-user values are semantically impossible: pg_doorman drops them with a single warning per pool and username and lets pool-level or general startup_parameters take over. The parsed map is stored on the credential cache entry. The next commit on this branch wires it into the cascade resolver.
ServerPool now owns a single Arc<QuarantineState> initialised from
general.startup_parameter_quarantine_{threshold,ttl}. On every backend
spawn the cascade (general / pool / passthrough auth_query) is resolved
lazily through pool::startup_resolver::resolve(); that lazy contract is
what allows general-level edits to take effect on the next backend
without forcing a pool-hash recycle.
Per-user parameters from auth_query enter the cascade only in
passthrough mode. In dedicated mode the cache layer already clears the
map (with an operator warning); this layer adds defence in depth by
refusing to consult the cache entry at all when the pool is dedicated.
After this commit, an operator config like [pools.app_db.startup_parameters]
plan_cache_mode = "force_custom_plan" actually reaches PG. The
prometheus counters and admin column ship in the next commit.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
added 16 commits
May 11, 2026 22:55
…alth * pg_doorman_backend_startup_parameter_errors_total (counter, labels pool / parameter / sqlstate) increments on every PG ErrorResponse that rejects an operator-supplied parameter at backend startup. The failing username is intentionally not a label so that dynamic auth_query pools (one Address per validated PG role) cannot blow up the series count when many roles use the same broken config; the username is in the corresponding warn log line. * pg_doorman_backend_startup_parameter_quarantined (gauge, labels pool / parameter) goes to 1 the moment a key is parked in the pool quarantine and back to 0 when its TTL expires. * SHOW POOLS gains a quarantined_params text column (comma-separated list) so operators can see which operator parameters a pool is currently dropping from StartupMessage. Replaces the temporary noop counter wrappers that were left in the backend startup error path while these vectors were being prepared.
Twelve scenarios exercise the operator-facing surface from this branch: cascade resolution across general, pool, and auth_query passthrough levels, reserved-key config rejection, the invalid-GUC backend startup error path, quarantine engagement after consecutive rejections, the central invariant that startup defaults survive a client-side RESET ALL or DISCARD ALL, RELOAD-driven pool recycle when pool.startup_parameters changes, dedicated-mode auth_query ignoring per-user values with a single warning, and the backwards-compatible path where auth_query SQL without the new column keeps working. Adds tests/auth_query_startup_params_fixture.sql with two users (sp_tuned_user with a per-user plan_cache_mode override in the JSON column, sp_plain_user with a NULL column value) used by the passthrough cascade, dedicated-mode warning, and baseline scenarios.
Phase 4 replaced handle_startup_error with an inline branch in Server::startup that always returned ServerStartupError. The pre-Phase-4 helper specifically mapped SQLSTATE 57P* (cannot_connect_now, admin_shutdown, crash_shutdown, database_dropped, etc.) to ServerUnavailableError, and only that error category triggers the Patroni-assisted fallback path. With the inline branch in place a backend reporting 57P03 no longer routed clients onto a healthy candidate; clients saw a startup error. Restores the classification: 57P* responses turn into ServerUnavailableError, every other ErrorResponse stays ServerStartupError. The startup_parameters quarantine and metrics side-effects still fire for the 22023 / 42704 / 42501 family on the way through; their evaluation is unrelated to the fallback decision.
Per-level validation in Config::validate caps general.startup_parameters and pool.startup_parameters separately. Two levels that each fit the 9 488-byte operator budget can together push the resolved cascade past PG's MAX_STARTUP_PACKET_LENGTH (10 000 bytes), at which point PG would reject every backend startup. With auth_query in the mix the per-pair syntax check sees only one entry at a time, so the per-user contribution to the budget is also invisible to load-time validation. resolved_startup_parameters now serialises the merged map and compares against the operator budget after the cascade has run. When the body would not fit, all operator-supplied keys are dropped for this spawn and the situation is logged; the backend connects with server defaults rather than the alternative of failing every connection attempt for this pool until the operator notices and trims the config. Adds serialized_bytes() as a pub helper and three unit tests: per-pair NUL accounting, empty-map zero, and the multi-level overflow case where each individual level fits but their union does not.
The startup-parameter quarantine fired whenever PG returned a sqlstate in the 22023 / 42704 / 42501 family and the parsed message named a parameter, regardless of whether that parameter was in the map pg_doorman sent. PG can produce those sqlstates for parameters pg_doorman does not control: an ALTER ROLE SET applied at login, a server-side default the role is not permitted to change, an internal extension, and so on. The quarantine and the counter would then record a rejection for a key that the operator did not configure, and on the next backend spawn pg_doorman would still try to send its own keys, producing a confusing mix in SHOW POOLS and metrics. The handler now cross-references the failing parameter name against the map it actually sent. If the key is ours, the same quarantine and metrics path as before runs. If it is not ours, pg_doorman logs an info line so the situation is visible and falls through to the regular ServerStartupError path without recording a rejection.
The rustdoc on BACKEND_STARTUP_PARAMETER_ERRORS_TOTAL still listed the counter as tracked "per pool/user/parameter" from an earlier draft. The label set is actually pool, parameter, sqlstate (the user was removed to keep series count bounded when dynamic auth_query pools mint many roles against the same broken config). Doc comment now matches the registered label set and notes where the username can be recovered for incident triage.
The quarantine counter incremented per rejection but never reset on success, so the threshold model effectively counted "N rejections ever" rather than "N consecutive rejections" the doc-comment and naming both promised. On a sea of healthy startups, a handful of transient failures spread over weeks could still tip a perfectly working parameter into quarantine. QuarantineState gains record_success(sent_keys); Server::startup calls it on ReadyForQuery for every operator-supplied key the spawn just accepted. The reset only touches keys that are not currently quarantined: TTL remains the sole release path for already-quarantined keys, because a quarantined key was by definition not in the sent map and so a later success says nothing about its underlying problem.
fields.yaml and the regenerated reference configs still promised that wire injection and the general / pool / auth_query cascade would arrive in a later commit. Both have shipped: pg_doorman puts the operator parameters into the StartupMessage on every backend spawn and resolves the cascade per call. An operator reading the auto-generated reference would have expected validation-only behaviour and been surprised when the new GUC actually showed up at PG. Doc strings now describe the shipped behaviour: cascade order, that values become pg_settings.reset_val and survive RESET ALL / DISCARD ALL, dedicated-mode auth_query exclusion, the post-cascade size check against MAX_STARTUP_PACKET_LENGTH, the quarantine path, and pointers to SHOW POOLS and the Prometheus counters for observability.
QuarantineState used to read its threshold and TTL once in ServerPool::new and never again. A SIGHUP that only changed general.startup_parameter_quarantine_threshold or _ttl had no effect on any pool that was kept across the reload, because the pool hash does not include general-level knobs and so the pool was reused as-is. Operators ended up running with the previous values until pg_doorman restarted or every pool was independently recycled, which made the runbook for tuning the quarantine misleading. QuarantineState now holds the two knobs as atomics. ServerPool exposes update_quarantine_knobs(threshold, ttl); the reload path pushes the fresh general-level values into every kept pool before declaring the pool unchanged. record_rejection reads the current values on every call. general.startup_parameters itself is read live from config_arc() on every backend spawn, so a SIGHUP that only changes those values takes effect on the next backend spawn for every pool without rebuild. Old idle backends keep their previous parameters until the regular pool recycle (server_lifetime / idle_timeout / RECONNECT) replaces them; this trade-off is intentional and documented in the operator notes.
…awns Before this change the BACKEND_STARTUP_PARAMETER_QUARANTINED gauge was only cleared inside Server::startup, when the next backend spawn ran filter_active_keys() and observed the expired entry. A pool that went idle right after quarantining a parameter (no further client traffic, nothing else triggers a spawn) left the gauge stuck at 1 for hours after the TTL was supposed to release the key, which would keep dashboards red and pagers loud long after the underlying issue had resolved. QuarantineState gains reconcile_expired(), and ServerPool wires it into the SHOW POOLS / metrics-collection path so every snapshot pass clears stale series. The reconciliation is a no-op for healthy pools and a constant-time operation per quarantined key, so calling it on every snapshot is cheap.
When the SELECT returns a native json or jsonb column named startup_parameters, tokio_postgres refuses to decode it as text and pg_doorman silently dropped the per-user parameters with a warn line that named the decode error but not the fix. Operators writing jsonb_build_object(...) AS startup_parameters would authenticate successfully, get an empty per-user map, and then chase the missing parameter through code paths that had nothing to do with the actual column type. The warn line now identifies the actual PostgreSQL type pg_doorman saw and recommends the smallest possible change to the SQL (`...::text AS startup_parameters`). The runtime contract stays the same: the column is read as text; conversion is the operator's responsibility.
Adds a tutorial covering the three-level cascade (general, per-pool, auth_query passthrough column), the RESET ALL invariant, validation at load and at each backend spawn, the quarantine with consecutive semantics and TTL-only release, and the observability surface (SHOW POOLS column plus the errors counter and quarantined gauge). The reference pages are already generated from fields.yaml; this is a curated walkthrough operators can follow before reaching for the reference. Both language books get the tutorial, linked from the Pooling section of SUMMARY.md next to the other per-pool behaviour articles.
The official postgres image starts an init listener, runs initdb scripts, then restarts before opening the final listener. pg_isready returns YES on the init listener, so the next query in the smoke (creating the smoke_user / smoke_db fixture) sometimes lands during the restart window and fails with FATAL: the database system is shutting down. That fail-pattern flaked the dashboard-validation workflow on every push to this branch with no underlying pg_doorman defect. Probe with two consecutive successful psql SELECT 1 runs instead of one pg_isready: if the first success was on the init listener, the second probe one second later catches the restart and the loop keeps waiting. The 90-second ceiling stays the same.
Bumps the package version to mark per-pool startup_parameters as the shipped feature of this release. The Changelog summarises the cascade, validation, quarantine, and observability surface introduced on this branch, and points to the new operator tutorial. The comparison page gains a row that contrasts the feature with the closest analogues in other poolers: PgBouncer only ships specific client-encoding/datestyle/timezone parameters per database via the connection string, Odyssey's maintain_params preserves client-side values across rebind but has no operator-side injection, and PgCat documents no equivalent. pg_doorman is the only one in this set that lets an operator put arbitrary GUCs into the backend StartupMessage on a per-pool basis.
The new feature file in tests/bdd/features/startup-parameters.feature ships with the @startup-parameters tag only. None of the existing BDD matrix entries pick that tag up, so the twelve end-to-end scenarios that exercise the feature were never running in CI even after the file landed. Locally the tag-filtered cargo invocation passes; the gap was only visible at the workflow level. Adds a dedicated matrix row that runs the same invocation in CI.
Operator visibility on operator-injected GUCs: * Admin SQL console: new SHOW STARTUP_PARAMETERS lists the per-pool effective merged cascade with the cascade layer that contributed each value (general / pool / auth_query) and a quarantined column. The entry is in the canonical SHOW_SUBCOMMANDS list, so psql tab completion on SHOW <TAB> now offers `startup_parameters`. * Web UI: /api/pools carries startup_parameters[] (with source per key) and quarantined_params[]. PoolDetail renders a "Startup parameters (operator-injected)" section that highlights quarantined keys, so an operator triaging a misbehaving pool no longer has to drop to psql to see which key is being parked. * BDD: new scenario "admin SHOW STARTUP_PARAMETERS lists the merged cascade per pool" under @startup-parameters. Codex review fixes: * HIGH #3 (SQLSTATE allowlist too narrow): quarantine now triggers whenever the parameter pg_doorman parses out of a PG ErrorResponse matches a key it actually sent, instead of restricting to the 22023 / 42704 / 42501 whitelist. The new heuristic covers 55P02 (cant_change_runtime_param) and any future or extension-specific codes a PG release reports under the startup family. SQLSTATE class 57P (server unavailable) keeps its dedicated handling so the Patroni-assisted fallback path stays unchanged. * HIGH #8 (final packet size guard checked the pre-final payload): validation now reuses the exact wire layout pg_doorman puts on the network - length prefix, protocol version, user, application_name, database, every operator-supplied pair, terminator - and rejects the cascade for one backend spawn when the full packet would exceed PG's 10000-byte cap. The backend then connects with PostgreSQL defaults and the operator sees a warn log naming the actual computed length. Operator documentation in EN and RU was overhauled to match the shipped behaviour: the RU startup-parameters tutorial got a full rewrite, RU reference docs gained the three new knobs, EN+RU auth-query docs document the new startup_parameters column, and the PgBouncer/Odyssey comparison rows in both languages now state the precise distinction (no operator-side injection of startup parameters into the backend StartupMessage, with the RESET/DISCARD contract pg_doorman provides). Reference configs and EN reference docs were regenerated to pick up the operator copy refresh in src/app/generate/fields.yaml.
…#5/#7/#9 Operator visibility was the right output; the cumulative-rejection quarantine that sat behind it was the wrong policy. PostgreSQL fails a connection when StartupMessage carries a parameter it cannot accept, and that is the contract operators reason about. The quarantine layer turned an honest PG error into a delayed retry with a TTL knob and a per-pool gauge — operator sees green tiles, client gets PG default, real failure stays hidden. This commit deletes it. What pg_doorman does on a PG-side startup-parameter rejection now: * SQLSTATE class 57P (server unavailable) still becomes ServerUnavailableError so the Patroni-assisted fallback path routes around a failed node before anything else. * Every other ErrorResponse becomes ServerStartupError; the client receives the same sqlstate and message it would have seen connecting to PG directly. No silent retry, no per-key park, no TTL. * When the failing parameter matches a key pg_doorman actually sent, the warn log line names the parameter, the SQLSTATE, and the username, and pg_doorman_backend_startup_parameter_errors_ total{pool, sqlstate} increments. The parameter name stays out of the Prometheus label set so a dynamic auth_query pool cannot blow up cardinality. Other codex review fixes folded into the same commit: * HIGH #2 — operator startup_parameters now win over client sync. Server tracks the operator-injected key set; sync_parameters on checkout drops those keys from the diff before issuing SET, so a client connect string that carries application_name (or any tracked GUC) no longer overrides the operator default on the backend. * HIGH #4 + #5 — RELOAD coherence on the operator baseline. The per-pool reuse hash now folds in general.startup_parameters, and the from_config path remembers the previous baseline hash so a SIGHUP that only edits general-level GUCs also recycles every dynamic auth_query pool that carried over the reload. Idle backends with the previous reset_val are drained, not stranded. * HIGH #7 — peek_startup_parameters now consults cache_ttl / cache_failure_ttl and returns None for expired entries, so the backend-spawn hot path cannot pin a stale per-user GUC after the operator updated the auth_query row. * HIGH #9 — parse_startup_parameters_text rejects raw column text larger than the operator budget before serde_json walks it, so a pathological auth_query row cannot CPU/heap-spike on every refresh. Admin SHOW STARTUP_PARAMETERS now exports five columns (user, database, parameter, value, source) instead of six; the quarantined column is gone with the rest of the machinery. SHOW POOLS drops the trailing quarantined_params text column. The Web UI pool detail page renders the same five-column view. Operator documentation in EN and RU loses the Quarantine sections; the startup-parameters tutorial now describes the PG-rejection contract end to end, with the new SHOW STARTUP_PARAMETERS example, and the comparison rows for PgBouncer/Odyssey were rewritten to match in both languages. Reference configs (pg_doorman.toml, pg_doorman.yaml) and EN reference docs were regenerated from src/app/generate/fields.yaml.
added 6 commits
May 12, 2026 12:51
Closes codex MED #9. The doc-comments on backend_startup_parameter_errors_total and startup_parameters_dropped_total claimed the `pool` label was a `<user>@<database>` identifier. Emit sites pass `address.pool_name`, which in the default mapping is the PostgreSQL database name - a multi-user database collapses into a single series, and the docs led operators to expect per-user attribution that never existed. State the actual contract: one series per pool name. Per-user attribution still lives in the warn log line that accompanies every counter increment.
Closes codex MED #15. CacheEntry.startup_parameters was a HashMap<String, String>, so every cache hit cloned the whole map. For a user whose auth_query row carries a dozen extension GUCs that is O(map) copies per connection (cache hit on get_or_fetch, again on every entry.clone() inside cache invalidation/refetch). The hot path lives under a DashMap shard read lock; a long clone there extends the lock window proportionally. Wrap the field in Arc<HashMap<...>>. Cache hits now do two atomic increments instead of a deep clone. dedicated_mode_filter drops the overlay by assigning a fresh empty Arc rather than .clear() — the existing Arc is shared with whoever already cloned the entry. Per-user overlay snapshot held on ServerPool is a separate Arc<BTreeMap>; that path was already zero-clone since the BLOCKER #2 fix. No change there.
…nels Closes codex MED #8. The three Row 19 panels selected on `instance, user, database`, but the underlying counters (pg_doorman_backend_startup_parameter_errors_total, pg_doorman_startup_parameters_dropped_total) carry only `pool` + `sqlstate`/`reason` labels. Any non-".*" value on the $user or $database template variable filtered the result to nothing, so even when the demo or a real deployment produced traffic the panels returned empty. The smoke-test allow_empty entry then masked the broken panel. Use an instance-only selector `SI` for the three panels and document the contract in the panel description ("filters on $user/$database do not apply"). The allow_empty entries stay legitimate for the demo (it ships no operator startup_parameters) but now the panels fill in the moment real traffic appears, regardless of the template variables' state.
…y map Closes codex MED #12. SHOW STARTUP_PARAMETERS and /api/pools rendered the configured cascade view (current general+pool+auth_query) without referencing the map pg_doorman actually ships in StartupMessage. Two real cases left a DBA looking at a misleading "active" entry: - The pool's frozen baseline / per-user overlay snapshot is behind the live config (RELOAD has not yet recycled, or the auth_query cache has not refetched). The next backend spawn still ships the old value. - The runtime budget/packet check dropped the entry on the most recent spawn (cascade_budget_exceeded / packet_cap_exceeded / auth_query_overlay_oversize); admin still showed it as configured. effective_startup_parameters_with_sources now cross-checks every configured entry against resolved_startup_parameters() and tags it with an ApplicationState: applied, dropped_due_to_budget, or stale. SHOW gains a `state` column, /api/pools' StartupParameterDto gains a `state` field next to `source`. Anonymous /api/pools still hides `value` per HIGH #5; `state` is operator-facing diagnostics, not config payload, so it stays in the anonymous view.
Closes codex LOW #16 (triple scan) and LOW #18 (HashSet per spawn). LOW #16. resolved_startup_parameters previously walked the merged cascade twice on the hot path - once through serialized_bytes for the operator budget and again through full_packet_bytes for the PG packet cap. The new packet_and_body_bytes helper returns both totals from one pass over the BTreeMap, halving the per-spawn map walk for the common no-overflow case and also for the baseline-only retry after auth_query overlay oversize. LOW #18. Server.operator_managed_startup_keys is now Arc<HashSet<String>>. Pools that ship no operator parameters - the overwhelming default - hand every spawn the same static Arc<HashSet::new()> instead of allocating a fresh empty HashSet per backend. Non-empty pools still allocate the set per spawn for now; the caller-side share is a follow-up because it requires threading the Arc through Server::startup signature.
Closes codex LOW #17. parse_startup_parameters_text built a one-entry BTreeMap around every (key, value) pair before passing it to startup_parameters:: validate(), just to reach the per-key/per-value checks inside. For a wide auth_query row that allocated and dropped one BTreeMap per key on cache miss / refetch. Expose validate_entry(&str, &str, &str) on the startup_parameters module and call it directly on the borrowed JSON pair. The output HashMap is now also sized to obj.len() up front, so wide rows do not pay the grow-and-rehash cycle either.
vadv
marked this pull request as ready for review
May 12, 2026 10:11
vadv
changed the title
added 11 commits
May 12, 2026 13:41
GitHub-hosted runners share CPU; with 20+ BDD suites fanning out the timing-sensitive scenarios (SCRAM passthrough reconnect, sleep-based retain windows) routinely lost their margin and reported a flake of the form `tls required but server does not support tls` on the fallback retry. The same scenarios pass deterministically locally and on master. Cap matrix concurrency at 4 so each suite gets a less contested runner; total wall-clock grows but each job stops racing itself off the runner.
Closes codex BLOCKER #1 (fresh review 2026-05-12). `RESERVED_KEYS` blocked `user`, `database`, `replication`, and `options` but let `role` and `session_authorization` through. Those two are not benign session defaults - they change PostgreSQL authorization state and become `pg_settings.reset_val` for the backend. After an operator injects `role` through `startup_parameters`, a client `RESET ROLE` restores the operator-injected role rather than the login role the pool authenticated under, which breaks the contract that `startup_parameters` carries only safe defaults. Add both keys to `RESERVED_KEYS` (case-insensitive comparison is already in place) and add regression tests for the lowercase and mixed-case spellings, plus tests against `validate_entry` so the auth_query JSON path also rejects them.
Closes codex HIGH #4 (fresh review 2026-05-12). /api/config flattened the active config and only masked keys whose last segment matched password/secret/token/key. startup_parameters values did not match that rule, so an anonymous viewer of the SPA or the public API could read every operator-supplied GUC value: tenant identifiers, audit routing tags, extension-specific GUC payloads, and any accidental secret. /api/pools already redacted this; /api/config slipped through. Add an is_startup_parameter_key() classifier that matches keys inside *.startup_parameters.* cascades. Make collect_config and handle_config role-aware: anonymous viewers see the parameter keys but receive *** in place of values; SSO and Admin keep the full view. Both `value` and `default` columns mask.
Closes codex HIGH #3 (fresh review 2026-05-12). The cold-pool auth path called get_server_parameters() and wrapped every PoolError into Error::ServerStartupReadParameters(err.to_string()), destroying the typed Error::ServerStartupParameterRejection that the create() / fallback paths carefully constructed with the real PG sqlstate and message. The first client to trigger cold-pool startup during authentication saw a generic 3D000 / 58000 instead of the real rejection - contradicting the "PG rejection forwarded verbatim" contract that the transaction-checkout path already honours. get_server_parameters() now special-cases PoolError::Backend (ServerStartupParameterRejection { .. }) and returns the typed error unchanged. All three auth callers (static user, dedicated auth_query, auth_query passthrough) detect the variant before the generic wrapper and forward sqlstate + message through error_response(), mirroring src/client/transaction.rs:773.
…drift Closes codex HIGH #2 (fresh review 2026-05-12). AuthQueryState reuse only checked the auth_query config and the pool-level startup_parameters hash. That excluded every other parent input the dedicated shared pool was built from: server_host, server_port, TLS material, application_name, timeouts, fallback settings, general.startup_parameters. A SIGHUP that edited any of these without touching the auth_query config let the old shared pool keep serving traffic, so dedicated-mode backends could outlive a RELOAD with stale TLS identity, stale reset_val, or pointed at the old backend host. Capture a parent fingerprint at state construction time: `pool_config.hash_value() ^ general_startup_hash`. The XOR folds in host/port/TLS/timeouts/fallback/app_name (covered by Pool's derived Hash) and the operator-wide baseline. RELOAD compares the fingerprint alongside the auth_query config and pool-level startup hash; a mismatch on any of the three drains dynamic pools, drops the cache, and lets the next auth rebuild the shared pool against the new parent.
…g halves Closes codex MED #6 (read-only views must not mutate metrics) and HIGH #5 (admin view must show wire-only keys). resolved_startup_parameters did two jobs in one function: it built the wire-ready map and it logged + incremented STARTUP_PARAMETERS_DROPPED_TOTAL on overflow. The admin and /api/pools views both went through this same function, so every SHOW STARTUP_PARAMETERS row in psql and every SPA poll of /api/pools could inflate the dropped counter and emit warn lines without any backend startup actually happening. Pull a pure classify_startup_parameters() out that returns the wire-ready Cow plus a BudgetDecision enum. resolved_startup_parameters becomes a thin wrapper that does the counter inc and the warn log only for the spawn path; effective_startup_parameters_with_sources (the admin/API view) calls the classifier directly with zero side effects. While we're here, fix HIGH #5: the admin view used to iterate only the configured cascade. If the pool's frozen baseline / per-user overlay still ships a key that the live config no longer mentions (RELOAD has not yet recycled the pool, auth_query cache has not refetched), that key was invisible. The view now iterates the union of configured ∪ wire and tags wire-only entries as Stale so the operator can see "backends are still sending plan_cache_mode even though I deleted it from config — RELOAD has not propagated yet."
…meters Closes codex MED #7 (fresh review 2026-05-12). ServerParameters::set_param canonicalizes timezone -> TimeZone and datestyle -> DateStyle so the in-memory map matches the casing PG sends back in ParameterStatus. operator_managed_startup_keys, however, stored the raw startup-map keys, so sync_parameters did an exact- string filter against the canonical-cased diff produced by compare_params. An operator value set as `timezone` left the canonical `TimeZone` unprotected: a client startup value reported as `TimeZone` would propagate through sync_parameters and overwrite the operator default, violating the "operator wins over client" rule. Extract canonicalize_param_name out of set_param into a pub helper and apply it when building operator_managed_startup_keys in Server::startup. Both spellings now collapse to the same canonical form before the filter runs.
Closes codex MED #8 (fresh review 2026-05-12). create_dynamic_pool re-peeked AuthQueryState's global cache to capture the per-user startup_parameters overlay even though the caller in auth/mod.rs had just fetched the row it authenticated against. With a low cache_ttl or a concurrent refetch, a user could authenticate against one row and have the dynamic pool built from a different (or missing) overlay snapshot. Take Arc<HashMap> as a parameter from the caller — `cache_entry.startup_parameters` in auth/mod.rs — and use it directly. The dedicated-mode guard stays for defence-in-depth, but the cache is no longer touched between authentication and pool construction.
GitHub-hosted runners share CPU across the matrix and across the broader runner pool, which produces two recurring flake classes: demo TPS warming up at a different pace per run, and timing-sensitive BDD scenarios (SCRAM passthrough reconnect after retain, sleep-based lifecycle waits) losing their margin when neighbouring jobs spike. Both repro deterministically locally and on master, so the runner share is the only signal that changed. Wrap dashboard-validate-ci in a 3-attempt loop with a `docker compose down -v` tear-down between attempts; the demo restarts clean, the panels get a fresh rate window, and the next attempt usually succeeds. Wrap each BDD suite in nick-fields/retry@v3 with max_attempts: 2 so a single timing-sensitive miss does not block the PR. Wall-clock impact is bounded because each suite is 1-3 min; retry only fires on actual failure, not as a baseline tax.
… metrics doc-comments Closes codex tech writer findings #17-22 (fresh review 2026-05-12): - SHOW STARTUP_PARAMETERS docs (en + ru) now list the `state` column alongside the existing `user|database|parameter|value|source` columns, with `applied | dropped_due_to_budget | stale` semantics explained in the tutorial. The BDD scenario "admin SHOW STARTUP_PARAMETERS lists resolved parameters per pool" pins the new column to `applied` so doc/runtime drift gets caught. - Oversize behaviour wording matches the current "drop the overlay if baseline alone fits; otherwise drop all operator-supplied keys" logic for both `cascade_budget_exceeded` and `auth_query_overlay_oversize`. - Russian Prometheus reference adds the `pg_doorman_startup_parameters_dropped_total` row with the full bounded `reason` enum. - Type-mismatch warning wording corrected from "one-time per user" to "for that fetched row", matching the actual log site. - SQLSTATE `57P*` prefix wording replaces the inaccurate "SQLSTATE class 57P" everywhere it appeared. - Dropped-counter wording uses "events" consistently in metric HELP text, RU prose, and the changelog. The src/web/metrics/mod.rs doc-comments and the auto-generated reference markdown were regenerated from fields.yaml as part of the same pass. The minor RU translation cleanup in unrelated tutorials (binary-upgrade, patroni-proxy, pool-pressure, etc.) fixes anglicisms picked up while comparing translations across the RU docs tree.
Closes codex MED #10 (fresh review 2026-05-12). The frontend assumed `value: string` always present. After the backend started masking values for anonymous viewers (HIGH #5 fix) React rendered "undefined" in the pool detail page; after the backend added the `state` field (MED #12 fix) the UI had no way to surface keys whose snapshot is stale or whose cascade overflows the budget. Update the TypeScript shape: `value?: string` (optional) and `state: string` (required, defaults to "applied" in the renderer). The list item shows the value or "***" for anonymous viewers, and prints the state inline next to the source label when state != "applied". Stale overlays render yellow; budget-overflow drops render red. Rebuild frontend/dist so the embedded bundle matches.
added 8 commits
May 12, 2026 14:32
A TLS-terminating reverse proxy in front of pg_doorman is common, but
the proxy → pg_doorman hop can ride a private HTTP leg. On shared or
multi-tenant networks an attacker on that segment can replay Bearer
JWTs or `sso_access_token` cookies pulled off the wire. Operators
need a way to make pg_doorman refuse those tokens unless the request
actually arrived over HTTPS.
New `[web].sso_require_https` knob, default `false`. When on, the
listener accepts Bearer/cookie/query SSO tokens only when the TCP
peer is in `trusted_proxies` AND the proxy forwarded
`X-Forwarded-Proto: https` (multi-hop chains must be `https`
end-to-end — any `http` segment downgrades). Plain-HTTP SSO attempts
yield 401 instead of silent Anonymous and bump
`pg_doorman_web_sso_validation_errors_total{reason="insecure_transport"}`,
so a misrouted operator notices instead of guessing. Basic auth is
unaffected — it lives or dies on the constant-time compare, regardless
of transport.
Default off keeps existing deployments working unchanged: the SSO
proxy still reaches pg_doorman over a private HTTP leg, no
configuration migration required. Documented in EN/RU web-ui guides
and surfaced in the annotated TOML/YAML references.
The previous sign-in modal was a 320px box tacked over the SPA: no visual anchor, the SSO button shared the chrome of the Basic form, and a returning operator could not tell at a glance which transport their browser was talking to. The new sign-in surface is a full-page console that reuses the JetBrains Mono / amber-tick / hairline-border idiom from the rest of the dashboard. SSO becomes the primary action with the proxy host shown inline so the operator knows where the redirect will route them; Basic is its own section below an `or use local admin` rule, keeping it discoverable without competing for attention. A transport chip in the footer reads `window.location.protocol` and colours itself green on `https:` (success) versus amber on `http:` (warning), so an operator about to send a Bearer JWT over plain HTTP sees the risk before they click. Accessibility tightened along the way: every input now has an explicit label-for/id pair on top of the existing focus trap, `role="dialog"` shell, and amber focus-visible ring inherited from the global stylesheet.
`/target` only anchored to the repo root, so `cargo build` inside `patches/openssl-src/` left `target/debug/.cargo-lock` showing up as untracked. Replace with `target/` so any cargo build directory under the tree is ignored. Also ignore `/docs/internal-plans/` alongside the existing `/docs/superpowers/` exclusion — same role: private session-scoped planning notes that should not enter public history.
…ing proxy The frontend redirect helper rejected any sso_proxy_url that did not start with `https://` (with a localhost escape hatch), logged `sso_proxy_url must use https` to the console, and left the "Sign in via SSO" button doing nothing. That contradicts the supported deployment shape where the corporate TLS-terminating proxy advertises an `http://` URL pointing at pg_doorman's own internal address, with TLS terminated upstream. Drop the protocol gate from `safeProxyUrl`. The URL still has to parse so a typo in `pg_doorman.toml` keeps showing in devtools; choosing http vs https stays with the operator. Backend `[web].sso_require_https` (default false) remains the explicit opt-in for environments that want pg_doorman to refuse plain-HTTP SSO tokens — that's the right place to enforce transport policy.
Two retries (max_attempts: 2) were not enough to recover from a GitHub Actions DNS outage to `index.crates.io` that lasted longer than cargo's own ~25-second network retry loop, draining both attempts in the same window (run #25734696524, Patroni proxy job failed twice in a row on `Could not resolve host: index.crates.io`). Three attempts plus a 30-second wait between them gives the DNS outage time to clear before the next attempt enters cargo's network loop. The third attempt only fires when both prior ones failed, so the timing-sensitive flake family (SCRAM passthrough reconnect, sleep-based lifecycle waits) does not pay an extra minute on single-attempt successes. Also drop the now-incorrect "no retry around the cargo step" header comment — retry has covered that step for several runs already.
…nnot wedge a job The bumped outer retry from the previous commit was treating a symptom: the BDD container kept failing with `Could not resolve host: index.crates.io` because the default docker bridge network does not always re-export the runner's systemd-resolved stub at 127.0.0.53. The container's resolver was simply broken for the whole job, so neither cargo's own internal retries nor the outer attempt-level retries had a path out — all three of cargo's tries kept hitting the same dead resolver in quick succession. Run the BDD container with `--network=host` so it inherits the runner's working `/etc/resolv.conf` directly, bypassing the bridge resolver entirely. Each matrix entry runs on its own ephemeral runner, so sharing the host network does not create loopback-port collisions between suites. While here, raise the cargo-internal envelope for the residual case where DNS works but crates.io rate-limits or returns 5xx mid-fetch: `CARGO_NET_RETRY=10` (default 2) gives cargo five times more attempts on its own network loop, and `CARGO_HTTP_TIMEOUT=60` (default 30 s) doubles the per-attempt ceiling. The outer 3-attempt retry stays as the last-resort safety net for whole-runner flakes.
… waiting longer The previous attempt bumped the outer step retry to 3×30 s on the assumption that the second attempt would land outside a transient DNS outage. Observation: when this fails, exactly one job in the matrix fails — the others on different ephemeral runners succeed at the same wall time. That rules out a fleet-wide DNS flake and points at a single runner whose bridge resolver is wedged for the whole job. A 30 s wait does not unwedge a dead resolver, so the extra attempts only stretch the failed wall time without changing the outcome. Restore the outer retry to the previous 2×5 s policy that exists only for the timing-sensitive BDD flake family (SCRAM passthrough reconnect, sleep-based lifecycle waits). The network defence now lives in two narrower places: `--network=host` short-circuits the bridge resolver so the container talks to the host's systemd- resolved stub directly, and `CARGO_NET_RETRY=10` / `CARGO_HTTP_TIMEOUT=60` widen cargo's own retry loop for residual crates.io flakes that arrive after DNS resolves cleanly.
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
2 participants
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
Add three-level cascade of PostgreSQL startup parameters (GUC injection) that pg_doorman sends to backends in the StartupMessage:
general.startup_parameters— baseline for all pools.pools.<name>.startup_parameters— per-pool override.auth_queryreturned JSON columnstartup_parameters— per-user override (passthrough mode only).Resolution: union of all levels, per-key the more specific level wins (auth_query > pool > general).
Motivation
plan_cache_mode = autocan stick to a generic plan after 5 EXECUTEs and become bad for skewed parameters. The blunt fix —ALTER ROLE/DATABASE SET plan_cache_mode = force_custom_plan— affects all workloads on that user/database. Reactive detection viapg_prepared_statements.generic_plans/custom_plansis non-trivial infrastructure. A proactive per-pool GUC mechanism is the architecturally clean answer, also coveringwork_mem,statement_timeout,lock_timeout, etc.Key design decision
PG documentation confirms: GUCs delivered via StartupMessage become
pg_settings.reset_val. ClientRESET ALL/DISCARD ALLdoes NOT remove them — PG restores them as session defaults. This invariant is the reason wire-method is startup-only (no post-auth SET fallback).