TML-3108: serialize queries per pinned pg client in the Postgres runtime driver - #29839
Conversation
Overlapping calls on a pinned pg.Client (direct driver, pinned connection, transaction handle) currently rely on pg@8 silently queueing them, which pg@9 replaces with a throw. Spy-based max-in-flight assertions and a process-warning capture document the required behavior: max 1 query in flight per physical client, FIFO order, pool-level paths unaffected. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
A WeakMap<client, AsyncMutex> serializes every unit of work on a physical client: the whole runQuery stream span, buffered query()/explain(), and BEGIN/COMMIT/ROLLBACK transaction boundaries. The per-client query lock is held across the entire BEGIN-to-COMMIT stream span, which subsumes the old streamSpanLocks; that second lock is deleted. Distinct PoolClients keep their own locks, so pool-level calls stay concurrent. Removes the reliance on pg@8's deprecated internal query queue, which pg@9 replaces with a throw. Documents the concurrency contract on PostgresBinding. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Holding the per-client lock across buffered-path row yields handed the lock lifetime to the consumer, deadlocking a nested query inside for-await on a cursor-disabled pinned client — the production path, and a shape that works under pg@8. The buffered path now locks only the awaited client.query; the cursor path keeps the wide stream-span lock (the cursor occupies the connection for its lifetime) and releases it before the buffered fallback. Consolidates the warning assertion into one dedicated test (util.deprecate fires once per process), renames the lease-overlap test to claim serialization rather than isolation, and documents lock lifetime on PostgresBinding. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…ient-serialization
|
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PostgreSQL driver now serializes operations per physical client using a shared mutex. Buffered queries lock only during execution, while cursor streams retain the lock through their lifecycle. Unit and integration tests validate ordering, transaction boundaries, and pool-client independence. ChangesPostgreSQL client serialization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Driver
participant ClientQueryLock
participant PhysicalClient
Driver->>ClientQueryLock: Acquire lock for physical client
ClientQueryLock->>PhysicalClient: Execute one query operation
PhysicalClient-->>ClientQueryLock: Complete query
ClientQueryLock-->>Driver: Release lock
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/3-targets/7-drivers/postgres/test/driver.pinned-client-serialization.integration.test.ts (1)
88-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the deprecation-warning filter to pg’s overlap warning.
captureProcessWarnings()currently stores every processDeprecationWarningemitted during the window, so unrelated deprecation warnings surface as this assertion failure. Use pg’s overlap-warning text in the filter, e.g. a case-insensitive pattern forCalling client.query when the client is already executing a query is deprecated and will be removed in pg@9.0. Use async/await or an external async flow control mechanism instead.🤖 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 `@packages/3-targets/7-drivers/postgres/test/driver.pinned-client-serialization.integration.test.ts` around lines 88 - 102, Update captureProcessWarnings to retain only DeprecationWarning instances whose message matches pg’s overlap warning text, using a case-insensitive pattern for the specified message; continue ignoring unrelated warnings while preserving the existing warnings collection and stop behavior.
🤖 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
`@packages/3-targets/7-drivers/postgres/test/driver.pinned-client-serialization.integration.test.ts`:
- Around line 56-86: Update createHarness to register the Harness with the
shared cleanup path before client.connect() and driver.connect() can fail, while
ensuring its close callback can access the initialized resources. Extend
Harness.close to close the pg Client after driver and server cleanup, preserving
cleanup when setup rejects and avoiding socket leaks.
---
Nitpick comments:
In
`@packages/3-targets/7-drivers/postgres/test/driver.pinned-client-serialization.integration.test.ts`:
- Around line 88-102: Update captureProcessWarnings to retain only
DeprecationWarning instances whose message matches pg’s overlap warning text,
using a case-insensitive pattern for the specified message; continue ignoring
unrelated warnings while preserving the existing warnings collection and stop
behavior.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 45af038b-a0fa-47d0-b521-3ff4a6688dca
📒 Files selected for processing (3)
packages/3-targets/7-drivers/postgres/src/postgres-driver.tspackages/3-targets/7-drivers/postgres/test/driver.pinned-client-serialization.integration.test.tspackages/3-targets/7-drivers/postgres/test/driver.pinned-client-serialization.test.ts
Register harness cleanup before the fallible connect steps so a rejection mid-setup still tears down the PGlite instance, socket server, and pg client, and end the pg client explicitly after driver/server shutdown (no-op-safe when the driver already ended it). Narrow the warning capture to pg's "already executing a query" DeprecationWarning so unrelated deprecations cannot flake the empty-warnings assertion. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
|
Both review items addressed in c90c1b3: harness cleanup now registers before the fallible connect steps and ends the pg client on close (socket-leak fix), and the warning capture is narrowed to pg's "already executing a query" DeprecationWarning so unrelated deprecations cannot flake the assertion. |
Verified every in-flight and not-started task against the codebase and Linear, and updated both roadmap surfaces (ROADMAP.md and ROADMAP.html) to match reality: - @db.* attribute deletion marked landed (TML-2988, #1054) - Repo move, v7 branch, and publishing pipeline statuses corrected (moved to prisma/prisma July 27-28; history graft dropped) - Scoreboard rewritten to the in-repo scorecard state (593 rows: 416/488/12/244); test-port count updated to 1,308 of 6,304 - Aggregate codec decoding marked resolved (TML-3064, #29867) - pg deprecation warning marked landed (TML-3108, #29839; TML-2628 closed) - Editor-support item rewritten to reflect the shipped language server - Added raw query support and deprecate-old-repo tasks; deleted the regenerate-examples task (regeneration happens per PR) - Recounted headers: 44 tasks, 14 done / 9 in flight / 21 not started Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Consider a Cloudflare Worker handler that fans out two independent reads on one serverless handle:
Both reads share one
pg.Client. Today this works because pg@8 silently queues the second query behind the first — a deprecated internal that pg@9 removes: a query sent while another is in flight will throw (maintainer-confirmed in node-postgres discussion #3598). pg only warns when a third call overlaps, so the common two-widePromise.allshape gives no warning at all today and becomes a hard error on the pg@9 upgrade.This PR makes the driver own that flow control instead: a per-physical-client FIFO mutex (a
WeakMapkeyed by the underlying client, reusing the existingAsyncMutex) serializes statements on every pinned-client path — direct{ kind: 'pgClient' }drivers, pinned connections fromacquireConnection(), and transaction handles, which covers the serverless facade and the Supabase runtime that inherits it. Pool-level calls are untouched: each checks out its ownPoolClient, so keying by physical client means they never contend.The lock is held for exactly as long as the client is protocol-busy, and no longer:
client.query(). Row iteration is lock-free, so a nested query on the same client insidefor awaitstill works. An earlier draft held the lock across the yields; review caught that it hands lock lifetime to the consumer and deadlocks that nested shape, confirmed by execution before narrowing.BEGIN…COMMITspan, because the cursor genuinely occupies the connection for its lifetime. This subsumes the previousstreamSpanLocks(deleted): the same mutex now guarantees concurrent streams cannot interleave their transaction wraps.BEGIN/COMMIT/ROLLBACK) each take the lock around their own statement — previously they bypassed all locking and could interleave into a concurrent statement.Semantics are unchanged for working code: FIFO order at statement granularity is exactly what pg@8's queue provided, including its one deadlock shape (awaiting a query inside iteration of a cursor stream on the same client), which remains and is now documented on
PostgresBinding. Statement-granularity locking deliberately does not fence other callers out of an open transaction on a shared client — that also matches pg@8, and no test claims otherwise.Tests were written red-first: max-in-flight spies read 3/4/2/3/2 overlapping queries across the direct driver, mixed methods, pinned connection, transaction, and lease-overlap cases before the fix (all now 1), and the integration harness (real
pg.Clientover a PGlite socket server) captured the pg deprecation warning verbatim before and none after. Guard tests pin the other direction: pool-level queries must stay concurrent, and four timeout-guarded regressions pin the nested-query/abandoned-generator/commit-during-stream shapes that the narrowed lock must not break. The warning assertion lives in a single dedicated test becauseutil.deprecatefires once per process.Changes are confined to
@prisma-next/driver-postgres(runtime driver + two test files). The CLI's control-plane driver needed no change — investigation on the linked ticket verified it already runs strictly sequentially (max 1 query in flight across a full migrate journey).Refs: TML-3108
🤖 Generated with Claude Code
Summary by CodeRabbit