feat(db): Postgres Db adapter — pg driver, pooler-safe schema scoping (HT-20) - #18
Conversation
… (HT-20) createPostgresDb wraps node-postgres behind the same Db/Queryable seam as PGlite. The schema option creates the schema if absent (tolerating the concurrent-create race and the pre-created-by-admin shape) and enforces search_path per-transaction via set_config(..., is_local => true), because session SET does not survive a transaction-mode pooler (Supabase 6543). query() in schema mode rides a single-statement transaction for the same reason. Uint8Array params are bridged to Buffer so bytea binds match PGlite. Tests drive the real pg driver over TCP against real Postgres (PGlite behind @electric-sql/pglite-socket) — no mocks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
- Default pool max to 2 (pg's 10 is per-instance-multiplied in serverless) - Verify USAGE+CREATE on the schema after ensure (missing USAGE silently drops the schema from search_path resolution — fail loud at boot instead) - Recheck schema existence after a swallowed create-race error rather than trusting the error code - Copy Uint8Array params into a fresh Buffer (no shared-memory view) - Document the commit-uncertain rejection window in the module doc Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Postgres-backed ChangesPostgres adapter
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PostgresDb
participant pgPool
participant pgClient
participant PostgreSQL
PostgresDb->>pgPool: connect()
pgPool->>pgClient: provide pooled client
PostgresDb->>pgClient: BEGIN
PostgresDb->>pgClient: set transaction-local search_path
PostgresDb->>pgClient: execute callback queries
PostgresDb->>pgClient: COMMIT or ROLLBACK
pgClient->>pgPool: release client
pgPool->>PostgreSQL: execute SQL
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db/index.ts`:
- Line 16: The public barrel export exposes PostgresDb and allows callers to
bypass createPostgresDb validation and provisioning. Update the export in
src/db/index.ts to expose only createPostgresDb, while keeping PostgresDb
available internally within the postgres module.
In `@src/db/postgres.test.ts`:
- Around line 230-246: Update the test “does not leak search_path onto pooled
connections used without schema” to construct both schema-scoped and schema-less
PostgresDb adapters over the same pg.Pool configured with max: 1, rather than
calling openDb() twice. Ensure the scoped operation completes before querying
through the plain adapter, and clean up the shared pool afterward so the test
verifies reuse of the same backend connection.
In `@src/db/postgres.ts`:
- Around line 119-133: Add a configurable connection acquisition timeout to the
pool options and pass it through when constructing the pool in the relevant
PostgreSQL client setup. Also register a pool error listener so idle-client
errors are handled and logged rather than becoming uncaught process-level
errors; update the connection options type and pool initialization symbols
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 685b51b4-e774-4c67-8e82-3d954e92ec14
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/db/index.tssrc/db/postgres.test.tssrc/db/postgres.ts
…sharper leak test
- Barrel exports only createPostgresDb; the PostgresDb constructor skips
schema validation/provisioning so it stays internal to src/db
- pool.on('error') handler: an idle client dying (pooler cull, backend
restart) is an uncaught 'error' event that crashes the process otherwise
- connectionTimeoutMillis defaults to 10s (pg's default waits forever)
- The search_path leak test now multiplexes a schema-mode and a plain Db
over ONE max-1 pool — the same physical session — so a regression from
transaction-local to session-local set_config would actually fail it
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/db/postgres.ts (1)
356-364: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmit a low-level log from the pool error handler.
The no-op avoids the crash, but it also hides idle-client failures from backend restarts or pooler recycling, which makes connectivity issues harder to diagnose. A single log line keeps the runtime behavior unchanged while preserving a signal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/postgres.ts` around lines 356 - 364, The pool error handler currently suppresses idle-client failures without recording them. Update the pool.on('error') handler in the pool initialization flow to emit one low-level log line containing the error details, while preserving the existing non-throwing behavior and avoiding additional recovery logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/db/postgres.ts`:
- Around line 356-364: The pool error handler currently suppresses idle-client
failures without recording them. Update the pool.on('error') handler in the pool
initialization flow to emit one low-level log line containing the error details,
while preserving the existing non-throwing behavior and avoiding additional
recovery logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b33ffb5-ad15-401e-9402-0bc087157720
📒 Files selected for processing (3)
src/db/index.tssrc/db/postgres.test.tssrc/db/postgres.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/db/postgres.test.ts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
What
A real Postgres implementation of the
Db/Queryableseam (src/db/postgres.ts), alongside the existing PGlite adapter. Closes the last code gap before the first deploy: the engine can now run against Supabase (or any Postgres) via a standard connection string.Jira: HT-20
Design
pg(node-postgres, MIT). Only unnamed prepared statements — compatible with transaction-mode poolers (Supabase Supavisor :6543, the serverless-correct connection).schemaoption contains every table in a dedicated Postgres schema (e.g.helpthreadinside a shared database) with zero changes to the stores' unqualified SQL. Enforcement is per-transaction (set_config('search_path', …, is_local => true)afterBEGINon a pinned client): a session-levelSETdoes not survive transaction pooling, where even consecutive autocommit statements can land on different backends.query()in schema mode rides a single-statement transaction for the same reason.ensureSchemahandles both production shapes: schema pre-created by an admin for a scoped role (existence checked first, no DDL attempted), or created on first boot (concurrent-create race tolerated, then existence re-checked). Afterwards it verifiesUSAGE+CREATEviahas_schema_privilege— a schema withoutUSAGEis silently skipped in search_path resolution, so mis-granted deploys fail loud at construction instead of with "relation does not exist" later.CREATE SCHEMADDL);pg_prefix rejected.maxdefaults to 2, not pg's 10 — serverless multiplies the default by warm-instance count.Uint8Arrayparams copied toBuffersobyteabinds behave identically to PGlite (pg would JSON-stringify a rawUint8Array).migrate()needed no changes: its advisory lock is alreadypg_advisory_xact_lock(transaction-scoped — the pooler-safe flavor), and it runs entirely insidedb.transaction(), so_migrationsand all tables land in the configured schema.Tests — real driver, real Postgres, no mocks
src/db/postgres.test.ts(20 tests) exposes a PGlite instance on a loopback TCP port via@electric-sql/pglite-socket(Apache-2.0, devDep) and connects with the actualpgdriver: wire-protocol parameter binding, commit/rollback (including a mid-transaction statement failure proving the pooled client is returned clean),bytearound-trip, schema placement verified through the catalog bypassing the adapter, cross-schema isolation, idempotent re-construction, and the real migrations run into a named schema. What the harness can't simulate — Supavisor's backend shuffling — is exactly what the transaction-local design defends against structurally (see module docs).247 tests total, typecheck + Biome clean.
Review
Codex adversarial pass (standing rule for the connection/DDL path): initial DO-NOT-SHIP with 5 findings (HIGH: pool default; MEDIUM: privilege validation, race postcondition; LOW: commit-uncertainty doc, zero-copy Buffer view) — all fixed in the second commit; re-review verdict SHIP, no new defects.
Licenses
pgMIT (dependency),@types/pgMIT +@electric-sql/pglite-socketApache-2.0 (devDependencies).🤖 Generated with Claude Code
https://claude.ai/code/session_01TqG66PPZreBrj17VbAqe3b
Summary by CodeRabbit
Uint8Array) parameter handling.