143 lines
7.1 KiB
Markdown
143 lines
7.1 KiB
Markdown
---
|
|
id: pg-fix-open-path
|
|
name: Fix `max_size: 0` open hang + DSN options override (review 002 Finding 3 + options note)
|
|
status: completed
|
|
depends_on: []
|
|
scope: single
|
|
risk: low
|
|
impact: component
|
|
level: implementation
|
|
tags: [wave-4-fixes, postgres-engine, open]
|
|
---
|
|
|
|
## Description
|
|
|
|
Two open-path fixes in `alkstore-postgres/src/store.rs`
|
|
(`open_store`), from the wave-4 general review:
|
|
|
|
**Finding 3 (MEDIUM, live-proven)**: `PgOpts { max_size: 0 }` hangs
|
|
`open` forever. Site: `store.rs:162-165` + the bootstrap `pool.get()`
|
|
at `store.rs:177` — deadpool (0.13/0.14) does not validate `max_size`
|
|
and defaults to no timeouts, so a `max_size: 0` pool can never grant a
|
|
checkout: `open` blocks forever at the bootstrap checkout (no
|
|
`BuildError`, no default pool timeouts) instead of failing typed.
|
|
Probe (removed): `open` with `max_size: 0` timed out at 5 s, never
|
|
returned.
|
|
|
|
Fix shape (the review's): validate `max_size > 0` in `open_store` —
|
|
typed `Error::Database` **before any round trip** (the engine-wide
|
|
validate-at-entry posture; the source-chain detail should say what was
|
|
rejected, e.g. "max_size must be positive"). The alternative shape
|
|
(configure deadpool timeouts so the hang surfaces as an error) is
|
|
weaker — a timeout-configured hang still burns the consumer's time;
|
|
prefer the guard. Do both only if trivial.
|
|
|
|
**Minor note (same file, same surface)**: `store.rs:152-156` —
|
|
`pool_cfg.options(...)` **replaces** any `options=` present in the
|
|
consumer's DSN (tokio-postgres's setter semantics) — a silent config
|
|
override. Preferred fix: **append** to the DSN's existing options
|
|
string (tokio-postgres `Config::get_options()` reads what the parse
|
|
carried) so the engine's `synchronous_commit` SET rides alongside
|
|
whatever the consumer configured. If appending proves impractical,
|
|
document at the site and in the crate docs that engine-configured
|
|
options win — but append is the honest default (the consumer's DSN
|
|
options were there first).
|
|
|
|
Tests (against the harness server): `max_size: 0` fails typed and
|
|
*promptly* (assert the error; the test's own structure bounds the
|
|
hang — a regression re-hangs and fails the test by timeout); a DSN
|
|
carrying pre-existing `options=` plus `open` succeeds and
|
|
`synchronous_commit` is still observable via `SHOW` on both settings
|
|
(the append path doesn't break the engine's SET); the existing
|
|
opts-flow tests stay green.
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [x] `max_size: 0` fails `open` with a typed `Database` error before
|
|
any round trip — pinned by test (prompt failure, no hang)
|
|
- [x] The consumer DSN's `options=` survive: appended, not replaced —
|
|
pinned by test (DSN with options + engine SET coexist, `SHOW`
|
|
observable)
|
|
- [ ] If append proved impractical instead: the engine-wins semantics
|
|
documented at the site and in crate docs (record the decision in
|
|
Notes) — *n/a: append landed*
|
|
- [x] Existing open/opts tests stay green
|
|
- [x] `cargo test -p alkstore-postgres` (harness server), clippy
|
|
`-D warnings`, fmt clean; gates green server-less
|
|
|
|
## References
|
|
|
|
- docs/reviews/002-wave-4-general-review.md §Finding 3 + §Minor notes
|
|
(the options bullet)
|
|
- docs/architecture/engine-postgres.md (Connection architecture)
|
|
- docs/architecture/decisions/008-contract-v1-pinning.md §6 (config
|
|
split)
|
|
- tasks/pg-engine-open-opts.md (the open task of record; Notes carry
|
|
the connect-options SET mechanics this task extends)
|
|
|
|
## Notes
|
|
|
|
Decisions of record made while implementing (the description didn't
|
|
pin them):
|
|
|
|
- **Both fix shapes' primary option landed (the guard; no deadpool
|
|
timeout config added)**: `open_store` rejects `opts.max_size == 0`
|
|
at entry — `database_error("max_size must be positive, got 0")`
|
|
(the value rides the message; the source chain says what was
|
|
rejected) — *before* the config parse and any round trip. The
|
|
timeout-configured-hang alternative was not taken (the review ranked
|
|
it weaker; not trivially worth doing both — deadpool 0.13/0.14 has
|
|
no default pool timeouts to lean on and the guard is total).
|
|
- **Append is practical and landed** (the task's preferred fix; the
|
|
n/a acceptance row): tokio-postgres 0.7.18's
|
|
`Config::get_options() -> Option<&str>` returns the raw options
|
|
string the parse (or a prior programmatic `options()`) carried, so
|
|
`open_store` reads it, appends
|
|
` -c synchronous_commit={on|off}`, and calls the setter — the
|
|
consumer's `options=` ride every pooled connection alongside the
|
|
engine's SET. Postgres parses repeated `-c` flags in order, so if
|
|
the consumer's own options set `synchronous_commit`, the engine's
|
|
later `-c` wins *for that knob* (it is the knob `PgOpts` owns) and
|
|
every other consumer setting lives untouched. The listener path
|
|
(`cfg.clone()` + `application_name` only) is unchanged — the SET is
|
|
a pool-connection concern. Documented at the site and on
|
|
`PgOpts::max_size` (0 rejected immediately) in `opts.rs`.
|
|
- **Tests**: `open_fails_database_promptly_on_zero_max_size` pins the
|
|
guard — typed `Database`, source chain containing "max_size" +
|
|
"positive", bounded by an inner `tokio::time::timeout(2s)` so a
|
|
regression re-hangs and fails here by timeout; it runs server-less
|
|
too (unreachable-DSN fallback — the guard fires before any
|
|
connection attempt, which is exactly the "before any round trip"
|
|
property being pinned). `consumer_dsn_options_survive_alongside_the_engine_set`
|
|
pins the append: DSN `options='-c statement_timeout=30000'` → open
|
|
succeeds, `SHOW statement_timeout` = `30s` (consumer's survived)
|
|
and `SHOW synchronous_commit` = on/off per `PgOpts` (engine's SET
|
|
rides) — both knob values asserted for both settings, against the
|
|
harness server.
|
|
- **The `PgOpts::max_size` doc on `opts.rs` now states the 0
|
|
rejection** (the constructor's validation is part of the option's
|
|
contract now that it is typed-failed at open).
|
|
|
|
## Summary
|
|
|
|
Both open-path fixes landed in `alkstore-postgres/src/store.rs`
|
|
(`open_store`). Finding 3: a `max_size > 0` entry guard fires before
|
|
the config parse and any round trip — `max_size: 0` now fails `open`
|
|
with the typed `Error::Database` ("max_size must be positive, got 0")
|
|
instead of hanging forever at the bootstrap checkout (deadpool
|
|
0.13/0.14 neither validates `max_size` nor defaults timeouts). Minor
|
|
note: the engine's connect-options SET (`-c synchronous_commit=…`) is
|
|
now *appended* to the parse-carried `options=` string (read back via
|
|
`tokio_postgres::Config::get_options()`) rather than replacing it —
|
|
the consumer's DSN options survive on every pooled connection
|
|
alongside the engine's knob; repeated `-c` flags are
|
|
order-processed by postgres so the engine still owns
|
|
`synchronous_commit` if it collides. Pinned by two new tests in
|
|
`src/store/open_tests.rs` (the zero-guard test with a bounded-hang
|
|
timeout structure, server-less-capable; the append test asserting
|
|
both `SHOW statement_timeout` and `SHOW synchronous_commit` across
|
|
both knob settings against the harness server). Verified: 118/118 pg
|
|
lib tests + contract-suite/schema suites green against the harness
|
|
(pglo-poc :15432), workspace `cargo test` green server-less (283
|
|
tests, pg skipping per convention), `cargo clippy --all-targets -- -D
|
|
warnings` and `cargo fmt --check` clean workspace-wide. |