feat: configure explicit Postgres connection pool and timeouts#114
Conversation
|
On it, @rubenhensen! I'm reviewing PR #114 (Postgres pool sizing and timeouts) for correctness and rule-compliance now — I'll post a consolidated review shortly. |
There was a problem hiding this comment.
Approve — clean, well-scoped change.
Review: intFromEnv correctly falls back on unset/empty/non-numeric/zero/negative/float input; the four postgres.js options (max, idle_timeout, connect_timeout, max_lifetime) are valid for postgres@3.4.x, and the .env.example defaults match the code (10 / 20 / 10 / 1800). Build (vite) OK and all 83 unit tests pass.
Rule compliance: conventional-commit title (feat:), non-draft, Closes #100 present, PR body matches the delivered diff. No violations found across the rule set.
Nit (non-blocking): a small unit test pinning intFromEnv's fallback branches would lock in the behaviour that was verified manually here — worth adding if you touch this again.
|
Reviewed and approved! Everything looks solid — conventional title, closes #100, intFromEnv fallbacks are correct, postgres.js option names check out for 3.4.x, and the .env.example defaults match the code. All 83 unit tests pass. One non-blocking nit in the review: it'd be nice to have a unit test covering the fallback branches of intFromEnv, but that's not a blocker. |
|
Thanks @dobby-coder — added |
Closes #100.
Gives the app's Postgres client explicit, env-tunable pool sizing and timeouts. Previously
src/lib/server/db/index.tsusedpostgres(env.DATABASE_URL)with library defaults (notably no connect/idle timeouts), and the pool couldn't be sized against Postgresmax_connectionsacross replicas.Changes
db/index.tsnow setsmax,idle_timeout,connect_timeout, andmax_lifetime, each overridable via env (DB_POOL_MAX,DB_IDLE_TIMEOUT,DB_CONNECT_TIMEOUT,DB_MAX_LIFETIME) with sensible defaults..env.exampledocuments the new optional knobs.Already covered
The other half of #100 — the migration script using a single short-lived connection — was already in place:
scripts/migrate.tsusespostgres(DATABASE_URL, { max: 1 })and callsclient.end(). No change needed there.