P0: Configure Postgres pool limits + statement_timeout (#165) - #182
P0: Configure Postgres pool limits + statement_timeout (#165)#182dkijania wants to merge 5 commits into
Conversation
58c7d49 to
dfcad11
Compare
|
Nice — this closes a real P0 cleanly, and isolating the option-building into a pure, unit-tested I checked the backwards-compat angle against the mina-explorer client and the defaults here are safe: 30s is generous for its heaviest path (a date-range analytics query that can fetch ~10k blocks), it's env-tunable, Two small, non-blocking things:
Thanks for tightening this up! |
Adds the integration coverage #165 actually asks for. The unit tests assert the options object's shape; these assert that Postgres really cancels a query past statement_timeout (SQLSTATE 57014), that the pooled connection stays usable afterwards — a cancelled query must not poison it, or one slow client would degrade later requests — and that PG_STATEMENT_TIMEOUT=0 disables the limit as documented. Also corrects PG_MAX_CONNECTIONS from "per host" to total. porsager's `max` is the whole pool, and a multi-host PG_CONN fails over rather than fanning out, so the pool only ever points at one host at a time. Since the README documents multi-host replicas, "per host" invited operators to size the pool N times too large. Addresses review feedback on #182. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @SanabriaRusso — both done in "per host" wording. Corrected in the docs table and the code comment; it's the total pool. Your reasoning checks out against the driver: (That same mechanism means the README's "the server fans queries across them" claim is wrong too — you flagged the equivalent in #197. Fixed on Acceptance criterion #1. Added await assert.rejects(
() => sql`SELECT pg_sleep(1)`,
(error) => error.code === '57014', // canceling statement due to statement timeout
);The client is built through They're also mutually confirming — the "inside the timeout" and "disabled" cases would still pass if the timeout silently weren't applied, but the |
|
Verdict: MERGEABLE ✅ Second pass focused on whether the defaults kill real production queries. They don't, and the two maintainer points from the first pass are genuinely addressed in What I checked
Non-blocking nits
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api. |
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving on the basis of the second-pass review comment above: no mid-to-high severity security, compatibility, or degradation issue found, and the downstream contract with mina-explorer / mina-explorer-api holds — GraphQL validation error text reaches errors[].message verbatim, the browser SPA's cross-origin access is preserved, and the real consumer query shapes (including the 2000-block analytics query and the 500-row page crawl) still pass.
Two things this approval does not mean:
- It does not close the non-blocking items in the review comment. Several are worth fixing before or shortly after merge; they are written up there with patches.
- It does not by itself mean the branch is ready to merge.
mainrequires branches to be up to date, so this needs an update-branch (or a rebase, if the branch is conflicting) first, and a few PRs in this series have cross-PR ordering constraints called out in their review comments.
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.
SanabriaRusso
left a comment
There was a problem hiding this comment.
Re-reviewed e986f4d ("fix(db): tighten statement timeout handling") after the earlier approval was dismissed. Both points from the previous review are addressed, and the fix is correct for the right reason. Re-approving.
The falsy-drop fix is genuine — verified against the pinned source
postgres@3.4.3, src/connection.js:972-982, builds the startup packet as:
Object.entries(Object.assign({ user, database, client_encoding: 'UTF8' }, options.connection))
.filter(([, v]) => v)
.map(([k, v]) => k + b.N + v)
.join(b.N)So .filter(([, v]) => v) is the exact mechanism: numeric 0 is falsy and was silently omitted from the packet, leaving the server default in force rather than explicitly disabling the timeout. String(0) → '0' is truthy and survives the filter. And because .map() string-concatenates the value anyway, stringifying is a no-op on the wire for every non-zero value — 15000 and '15000' serialise to identical bytes. No behaviour change except the one intended.
The as unknown as number cast is necessary, and the comment explaining it is accurate
postgres@3.4.3 types/index.d.ts:335 declares statement_timeout: number as an explicit key, which wins over the [name: string]: string | number | boolean index signature two lines below at :342. So a string genuinely cannot be assigned without the cast. This is an upstream typing inaccuracy — the value is serialised as a string regardless — and the four-line comment says so correctly. Fine as written; not worth contorting the code around.
Default 15000 adopted
Documented rationale in docs/getting-started.md:181 is the right one: keeping the server-side cancel below the 20s client timeout means Postgres reclaims the work before the caller abandons it, instead of continuing to burn a connection on a query nobody is waiting for. That matters specifically because mina-explorer-api retries transport timeouts twice but never retries GraphQL errors (app/upstream/graphql.py), so a 30s server-side timeout under a 20s client timeout turned one slow scan into three concurrent ones.
Consistency checked across the whole PR: .env.example.compose, .env.example.lightnet, docs/getting-started.md, and POOL_DEFAULTS all read 15000, with no stale 30000 anywhere in the diff. Sibling docs PRs are unaffected — #197 names PG_STATEMENT_TIMEOUT without quoting a value, and #186/#196 don't reference it.
One residual worth measuring after rollout, not a blocker: 15s now sits below the consumers' 20s client timeout, so any query landing in the 15–20s band flips from slow-but-successful to a hard error. The most plausible candidate is the explorer's 2000-block analytics query on mainnet (ANALYTICS_BLOCK_LIMIT = 2000). The README benchmark (~800 req/s, p99 39ms) says ordinary traffic is nowhere near this, and the value is env-configurable so an operator can raise it — but it's worth timing that one query against mainnet once this is deployed, rather than assuming.
Non-blocking: the new integration test cannot fail
tests/integration/postgres-timeout.test.ts — 'PG_STATEMENT_TIMEOUT=0 sends an explicit session-level disable' asserts SHOW statement_timeout returns '0'. But a stock Postgres already defaults statement_timeout to 0, so this passes whether or not the parameter was ever transmitted — which is the same vacuity the previous review flagged in the test above it. It would only discriminate against a server with a non-zero default:
ALTER DATABASE archive SET statement_timeout = 5000;...set before the client connects, then asserting '0' still wins.
Worth noting the regression is nonetheless genuinely covered — by the unit test, not the integration test. tests/unit/postgres-options.test.ts:64 uses assert.deepStrictEqual(options.connection, { statement_timeout: '20000' }), and deepStrictEqual distinguishes '20000' from 20000, so reverting the String(...) fails that assertion. So this is a cleanup suggestion — either make the integration test discriminating or drop it as redundant — not a coverage gap.
Automated re-review of e986f4d — focus: downstream compatibility with mina-explorer / mina-explorer-api.
SanabriaRusso
left a comment
There was a problem hiding this comment.
Correcting my previous review on this PR: I approved on the strength of the production change before checking CI, and Run-Tests — a required check — is red on e986f4d. The failure is caused by the test added in that same commit, so this needs one more push before it can merge. Everything I said about the production fix still stands; only the new test is wrong.
The failure
not ok 5 - PG_STATEMENT_TIMEOUT=0 sends an explicit session-level disable
location: 'tests/integration/postgres-timeout.test.ts:91:3'
error: |-
Expected values to be strictly equal:
undefined !== '0'
Cause
SHOW statement_timeout returns a single column named statement_timeout. The test reads row.t:
const [row] = await sql<{ t: string }[]>`SHOW statement_timeout`;
assert.strictEqual(row.t, '0');The <{ t: string }[]> annotation is a compile-time claim that nothing validates at runtime, so row.t is undefined and the assertion fails. SHOW does not accept an AS alias, which is presumably how t was intended to be produced.
Fix
current_setting() is the better instrument here, because unlike SHOW it is an ordinary function call and so does accept an alias:
test('PG_STATEMENT_TIMEOUT=0 sends an explicit session-level disable', async () => {
const sql = postgres(
connectionString,
buildPostgresOptions({ PG_STATEMENT_TIMEOUT: '0' })
);
try {
const [row] = await sql<{ t: string }[]>`
SELECT current_setting('statement_timeout') AS t
`;
assert.strictEqual(row.t, '0');
} finally {
await sql.end();
}
});One more thing, while you are in this file
Once the column name is corrected, this test will pass — but it would also have passed before the String(...) fix, because a stock Postgres already defaults statement_timeout to 0. So it does not actually discriminate the bug it was written to pin: omitting the parameter and explicitly sending 0 produce the same observable result on a default server.
To make it discriminating, give the server a non-zero default first, so that only an explicitly transmitted 0 can win:
test('PG_STATEMENT_TIMEOUT=0 overrides a non-zero server default', async () => {
const admin = postgres(connectionString);
try {
await admin`ALTER DATABASE ${admin.unsafe(database)} SET statement_timeout = 5000`;
} finally {
await admin.end();
}
const sql = postgres(
connectionString,
buildPostgresOptions({ PG_STATEMENT_TIMEOUT: '0' })
);
try {
const [row] = await sql<{ t: string }[]>`
SELECT current_setting('statement_timeout') AS t
`;
assert.strictEqual(row.t, '0');
} finally {
await sql.end();
const cleanup = postgres(connectionString);
try {
await cleanup`ALTER DATABASE ${cleanup.unsafe(database)} RESET statement_timeout`;
} finally {
await cleanup.end();
}
}
});ALTER DATABASE ... SET only affects sessions opened after it runs, which is why the admin connection is closed before the assertion connection is opened.
If that feels heavier than it is worth, the alternative is to delete this integration test entirely — the regression is already genuinely covered at the unit level. tests/unit/postgres-options.test.ts:64 asserts:
assert.deepStrictEqual(options.connection, { statement_timeout: '20000' });and deepStrictEqual distinguishes '20000' from 20000, so reverting the String(...) fails that assertion with no database required. Either route is fine; the current test is the one option that does not work.
Unchanged from my previous review
The production fix is correct and I verified it against the pinned source — postgres@3.4.3 src/connection.js:979 filters startup parameters with .filter(([, v]) => v), so numeric 0 was dropped and '0' survives, while non-zero values serialise to identical wire bytes. The as unknown as number cast is genuinely required (types/index.d.ts:335 narrows that key to number, overriding the index signature at :342) and the explanatory comment is accurate. The 15000 default and its documented rationale are right, with the residual note that 15s now sits below the consumers' 20s client timeout, so it is worth timing the 2000-block analytics query against mainnet after rollout.
Happy to re-approve as soon as the test is fixed.
Automated re-review of e986f4d — focus: downstream compatibility with mina-explorer / mina-explorer-api.
Adds the integration coverage #165 actually asks for. The unit tests assert the options object's shape; these assert that Postgres really cancels a query past statement_timeout (SQLSTATE 57014), that the pooled connection stays usable afterwards — a cancelled query must not poison it, or one slow client would degrade later requests — and that PG_STATEMENT_TIMEOUT=0 disables the limit as documented. Also corrects PG_MAX_CONNECTIONS from "per host" to total. porsager's `max` is the whole pool, and a multi-host PG_CONN fails over rather than fanning out, so the pool only ever points at one host at a time. Since the README documents multi-host replicas, "per host" invited operators to size the pool N times too large. Addresses review feedback on #182. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e986f4d to
eebd4e3
Compare
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving. Both round-1 findings are fixed, and I verified the second one empirically rather than by inspection.
The SHOW column-name bug is fixed. tests/integration/postgres-timeout.test.ts:136-138 now reads SELECT current_setting('statement_timeout') AS t. Confirmed against a real Postgres 15 that SHOW statement_timeout returns a column named statement_timeout (hence the old undefined !== '0') while current_setting(...) AS t returns { t: ... }. Run-Tests is green and the log shows the subtest actually running, not skipped.
The test is now genuinely discriminating — this was my main concern from round 1, so I reproduced both arms against a throwaway Postgres using the repo's own postgres@3.4.3:
connection.statement_timeout |
current_setting('statement_timeout') |
assertion |
|---|---|---|
'0' (current code) |
"0" |
passes |
0 (the pre-fix regression) |
"1s" |
fails |
The mechanism is exactly right: setDatabaseStatementTimeout('1s') applies ALTER DATABASE … SET on a separate max:1 client that is await sql.end()-ed in a finally before the assertion connection is created, so the assertion session inherits the non-zero database-level default unless the startup packet explicitly overrides it. Cleanup is safe too — resetDatabaseStatementTimeout() runs in finally, and setupTestDatabase() recreates the DB per run regardless.
Production change re-verified after the rebase. postgres@3.4.3 src/connection.js:980 still builds the startup packet with .filter(([, v]) => v), so numeric 0 is still dropped — the String(...) at src/db/archive-node-adapter/postgres-options.ts:85 is load-bearing, and the as unknown as number cast is still required by the upstream .d.ts. 15000 is consistent across .env.example.compose, .env.example.lightnet, docs/getting-started.md:181, and POOL_DEFAULTS; a repo-wide grep finds no stale 30000.
Good catch on the .gitignore fix, which is load-bearing here rather than incidental: on main, git check-ignore -v src/db/newfile.ts reports .gitignore:27:db/, i.e. every new file under src/db/ — including this PR's own postgres-options.ts — was being silently ignored.
Non-blocking notes:
- Merge ordering. #134 touches the same
.gitignorehunk (it anchors/db/but leavesdata/unanchored) and #187 touches the samepostgres(connectionString)line inarchive-node-adapter.ts:44. Whichever lands second needs a trivial conflict resolution; prefer this PR's fully-anchored.gitignore. For thearchive-node-adapter.tscollision, resolve tothis.client = postgres(connectionString, buildPostgresOptions());and leave #187'spingClienton its own literal options — passingbuildPostgresOptions()to the ping client would give itmax: 10and undo the pool isolation #187 exists to provide. - Measure after rollout. 15s now sits below the ~20s client timeout used by both known downstream consumers (
mina-explorersrc/services/api/http.ts:9DEFAULT_TIMEOUT_MS = 20_000;mina-explorer-apiapp/config.py:91upstream_timeout_seconds = 20.0). Any query in the 15–20s band flips from slow-but-successful to a hard error. The plausible candidate is the explorer's 2000-block mainnet analytics query — worth timing against mainnet rather than assuming.PG_STATEMENT_TIMEOUTis env-tunable if it turns out to be over. - A cancelled query surfaces as SQLSTATE 57014 and is masked into a generic error. Confirmed it does not contain
Cannot query field,Unknown argument, orUnknown type— the three literal stringsmina-explorer-apimatches on atapp/upstream/graphql.py:33-42to drive tier fallback and poison its capability cache. So a statement timeout cannot be misread downstream as a permanent schema error. Good. PG_IDLE_TIMEOUT=0/PG_CONNECT_TIMEOUT=0are accepted and mean "no timeout" to postgres.js (onlymaxis clamped). Fine as an operator opt-in, just noting it's asymmetric with themaxclamp.
Adds the integration coverage #165 actually asks for. The unit tests assert the options object's shape; these assert that Postgres really cancels a query past statement_timeout (SQLSTATE 57014), that the pooled connection stays usable afterwards — a cancelled query must not poison it, or one slow client would degrade later requests — and that PG_STATEMENT_TIMEOUT=0 disables the limit as documented. Also corrects PG_MAX_CONNECTIONS from "per host" to total. porsager's `max` is the whole pool, and a multi-host PG_CONN fails over rather than fanning out, so the pool only ever points at one host at a time. Since the README documents multi-host replicas, "per host" invited operators to size the pool N times too large. Addresses review feedback on #182. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
28d28ba to
efae1b6
Compare
Adds the integration coverage #165 actually asks for. The unit tests assert the options object's shape; these assert that Postgres really cancels a query past statement_timeout (SQLSTATE 57014), that the pooled connection stays usable afterwards — a cancelled query must not poison it, or one slow client would degrade later requests — and that PG_STATEMENT_TIMEOUT=0 disables the limit as documented. Also corrects PG_MAX_CONNECTIONS from "per host" to total. porsager's `max` is the whole pool, and a multi-host PG_CONN fails over rather than fanning out, so the pool only ever points at one host at a time. Since the README documents multi-host replicas, "per host" invited operators to size the pool N times too large. Addresses review feedback on #182. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
efae1b6 to
58da1bb
Compare
SanabriaRusso
left a comment
There was a problem hiding this comment.
Re-approving after the rebase (the previous approval was dismissed by the force-push).
Re-verified rather than assumed: the PR content at 58da1bb is byte-identical to the commit I approved (28d28ba) once both are diffed against their respective merge bases. Every +/- line matches. The rebase onto 7361526 was mechanical.
Carrying forward the round-2 verification unchanged:
- The failing test now reads
SELECT current_setting('statement_timeout') AS t, and it genuinely discriminates the bug.ALTER DATABASE ... SET statement_timeout='1s'is applied on a separatemax:1client that isend()-ed before the assertion connection opens, so the assertion session inherits the non-zero DB default. Both arms reproduced against a real Postgres with the repo's ownpostgres@3.4.3:statement_timeout: '0'→"0"(passes);statement_timeout: 0→"1s"(fails). - Production fix re-confirmed at
connection.js:980—.filter(([,v]) => v). - All checks green on this head.
Residual (measure, not a blocker): 15s sits below both consumers' 20s client timeout, so a query in the 15–20s band flips from slow-but-successful to a hard error. Time the explorer's 2000-block mainnet analytics query post-merge; PG_STATEMENT_TIMEOUT is env-tunable.
Merge-train note: this PR and #134 both fix the unanchored .gitignore:27 db/ entry independently and will conflict on that hunk. Resolve toward the fully anchored /db/ + /data/ form.
The archive-node Postgres client was created with `postgres(connectionString)` and no pool sizing or timeouts. With the API exposed publicly this is a denial- of-service risk: one expensive query can hold a connection open indefinitely, exhausting the pool and cascading into an outage. Add a small, unit-testable `postgres-options` module that builds the client options from conservative, env-tunable defaults: - PG_MAX_CONNECTIONS (max pooled connections, default 10) - PG_IDLE_TIMEOUT (seconds, default 30) - PG_CONNECT_TIMEOUT (seconds, default 30) - PG_STATEMENT_TIMEOUT(ms server-side query cap, default 30000; 0 disables) Malformed values fall back to defaults rather than throwing, so a stray typo can never silently disable a safety limit. Docs, env example, and env type declarations updated; unit tests cover parsing, fallbacks, and the options shape. Also anchor the `db/` and `data/` .gitignore rules to the repo root (`/db/`, `/data/`). The unanchored `db/` matched `src/db/` anywhere in the tree, which silently ignored the new module file. Closes #165. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSuak9smCHbp4N17xjjLF6
Adds the integration coverage #165 actually asks for. The unit tests assert the options object's shape; these assert that Postgres really cancels a query past statement_timeout (SQLSTATE 57014), that the pooled connection stays usable afterwards — a cancelled query must not poison it, or one slow client would degrade later requests — and that PG_STATEMENT_TIMEOUT=0 disables the limit as documented. Also corrects PG_MAX_CONNECTIONS from "per host" to total. porsager's `max` is the whole pool, and a multi-host PG_CONN fails over rather than fanning out, so the pool only ever points at one host at a time. Since the README documents multi-host replicas, "per host" invited operators to size the pool N times too large. Addresses review feedback on #182. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
58da1bb to
69cbf2f
Compare
What & why
Part of the production-readiness epic (#163). Closes #165.
The archive-node Postgres client was created with
postgres(connectionString)and no pool sizing or timeouts. Once the API is publicly reachable this is a DoS risk — a single expensive query can hold a connection open indefinitely, exhaust the pool, and cascade into an outage.Changes
src/db/archive-node-adapter/postgres-options.ts— builds thepostgres()options from conservative, env-tunable defaults, isolated so it's unit-testable without a DB.postgres(connectionString, buildPostgresOptions()).PG_MAX_CONNECTIONS10PG_IDLE_TIMEOUT30PG_CONNECT_TIMEOUT30PG_STATEMENT_TIMEOUT300000disablesMalformed values fall back to defaults rather than throwing, so a stray typo can never silently disable a safety limit (e.g.
max→ 0).statement_timeoutis sent as a startup connection parameter, so it applies to every query on every connection.Docs (
getting-started.md),.env.example.compose, andenvionment.d.tsupdated; new unit tests cover parsing, fallbacks, clamping, and the options shape.Testing
npm run build— cleannpm run test:unit— all pass (6 new assertions inpostgres-options.test.ts)npm run lint— cleannpx prettier --debug-check .— exit 0Tests construct their own
postgresclients directly, so the new adapter defaults don't affect the integration/live-network suites.🤖 Generated with Claude Code