fix(adapter-pg): serialize queries within a transaction - #29468
fix(adapter-pg): serialize queries within a transaction#29468matingathani wants to merge 7 commits into
Conversation
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded PostgreSQL TIMETZ array type parsing and serialized PgTransaction query execution with a mutex; also added the async-mutex runtime dependency. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
There was a problem hiding this comment.
Pull request overview
Fixes a runtime concurrency issue in client-engine-runtime where join relation fetches were executed concurrently inside transactions, triggering the pg deprecation warning (and future pg@9 error) caused by overlapping client.query() calls on a single transaction connection.
Changes:
- Detect when the interpreter is executing with a transactional
SqlQueryable. - Run
joinchildren fetches sequentially inside transactions while keeping the parallelPromise.allbehavior outside transactions. - Add an
isTransactiontype guard to support the conditional behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Since this is a |
|
Thanks for the direction — agreed, the adapter is the right place for this. I looked at how That approach is strictly better than what I did in the runtime: the runtime doesn't need to know whether a queryable can handle concurrent calls, and keeping it pg-specific means other adapters are unaffected. I'll update the PR to revert the |
When `update` (or any write) with `include` relations is executed inside a transaction, the `join` node in the query interpreter fires a `Promise.all` over all child relation fetches. Each child eventually calls `queryRaw` on `context.queryable`, which — inside a transaction — is a single `pg.PoolClient` (not a pool). Concurrent `client.query()` calls on a single pg Client trigger: "Calling client.query() when the client is already executing a query is deprecated and will be removed in pg@9.0." This becomes a hard error in pg@9.0. Fix: detect whether `context.queryable` is a `Transaction` (via the `commit` property on the `Transaction` interface) and, if so, fetch the join children sequentially instead of in parallel. When using a connection pool the existing parallel behaviour is preserved. Fixes prisma#29407
dee0259 to
b19be18
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/adapter-pg/src/pg.ts (1)
138-159: 🧹 Nitpick | 🔵 TrivialMutex-based serialization correctly addresses the root cause.
Moving the fix from the runtime (
query-interpreter.ts) into the adapter is the right layering: the runtime stays agnostic of connection concurrency semantics, and other adapters (which already handle this with their own mutex, e.g. PlanetScale) remain unaffected. Thetry/finallyguaranteesrelease()runs even whensuper.performIOthrows viaonError, so the mutex cannot be permanently held after a failed query.One optional simplification:
async-mutexexposesrunExclusivewhich removes the need for the explicitacquire/try/finallyboilerplate:♻️ Optional refactor using
runExclusive- protected async performIO(query: SqlQuery): Promise<pg.QueryArrayResult<any>> { - const release = await this.#mutex.acquire() - try { - return await super.performIO(query) - } finally { - release() - } - } + protected async performIO(query: SqlQuery): Promise<pg.QueryArrayResult<any>> { + return this.#mutex.runExclusive(() => super.performIO(query)) + }Note: unlike
PlanetScaleTransaction.performIO, there's intentionally nocatchhere becausesuper.performIO(inPgQueryable) already funnels errors throughthis.onError, which throws aDriverAdapterError. Re-catching would double-wrap the error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/adapter-pg/src/pg.ts` around lines 138 - 159, The current PgTransaction.performIO uses explicit acquire/release on the `#mutex` which is correct but can be simplified: replace the acquire/try/finally pattern in PgTransaction.performIO with async-mutex's runExclusive to serialize calls, i.e., call this.#mutex.runExclusive(() => super.performIO(query)) so you still serialize access on the pg.PoolClient while removing manual release boilerplate; keep the behavior that errors are not re-caught (super.performIO in PgQueryable still funnels errors through this.onError).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/adapter-pg/src/pg.ts`:
- Around line 138-159: The current PgTransaction.performIO uses explicit
acquire/release on the `#mutex` which is correct but can be simplified: replace
the acquire/try/finally pattern in PgTransaction.performIO with async-mutex's
runExclusive to serialize calls, i.e., call this.#mutex.runExclusive(() =>
super.performIO(query)) so you still serialize access on the pg.PoolClient while
removing manual release boilerplate; keep the behavior that errors are not
re-caught (super.performIO in PgQueryable still funnels errors through
this.onError).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b66f5fb0-95ed-40a4-956d-23ea12472b1c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/adapter-pg/package.jsonpackages/adapter-pg/src/conversion.tspackages/adapter-pg/src/pg.ts
|
@jacek-prisma could you approve the CI run when you get a chance? The workflows are blocked waiting for maintainer approval. |
Replace the manual acquire/try/finally/release pattern in PgTransaction.performIO with async-mutex's built-in `runExclusive`, which handles acquire and release internally. Semantically identical to the previous implementation — serialises all performIO calls on the underlying pg.PoolClient — but removes boilerplate and avoids any risk of a missed release on unexpected throws. Addresses CodeRabbit suggestion on PR prisma#29468.
|
Addressed in the latest commit (7765742) — replaced the manual |
|
Looks good, but I think the 4a9f3df commit is unrelated and shouldn't be in this PR |
|
Good catch — that commit was unintentionally included. I'll squash it out in the next push. |
pg.PoolClient does not support concurrent client.query() calls on a single connection. Inside a transaction all relation-join child queries were fired via Promise.all, hitting this limitation and triggering the pg deprecation warning (hard error in pg@9.0). Move the fix to the adapter layer, matching the pattern already used in adapter-mssql and adapter-planetscale: PgTransaction overrides performIO and gates each call behind a Mutex from async-mutex, so concurrent callers are serialised at the connection level without touching the runtime. Revert the query-interpreter.ts change from the previous commit; the runtime no longer needs to be aware of whether a queryable is a transaction. Fixes prisma#29407
Replace the manual acquire/try/finally/release pattern in PgTransaction.performIO with async-mutex's built-in `runExclusive`, which handles acquire and release internally. Semantically identical to the previous implementation — serialises all performIO calls on the underlying pg.PoolClient — but removes boilerplate and avoids any risk of a missed release on unexpected throws. Addresses CodeRabbit suggestion on PR prisma#29468.
7765742 to
0be9563
Compare
|
Done — removed the unrelated commit. The branch now only contains the mutex-based serialization fix. |
…rent-transaction-queries
tensordreams
changed the title
|
Merged latest The mutex is scoped to |
|
We're hitting this too, and can add two data points that widen the scope of the bug. 1. It's not limited to This fires the warning on every call: const [courses, totalCount] = await prisma.$transaction([
prisma.course.findMany({
where,
include: {
teacher: { select: { name: true } },
schedules: true,
coursePriceAdjustment: { // nested include with per-parent take
include: { adjustment: true },
orderBy: { adjustment: { appliedAt: "desc" } },
take: 1,
},
},
skip,
take: pageSize,
}),
prisma.course.count({ where }),
]);Captured with The 2. The "downgrade pg" workaround doesn't work. The deprecation warning is not new in pg 8.20 — we reproduced it on Confirming the fix in #29468 works: we applied an equivalent local patch via // PgTransaction
constructor(...) { ...; this.queryChain = Promise.resolve(); }
async performIO(query) {
const result = this.queryChain.then(() => super.performIO(query));
this.queryChain = result.then(() => {}, () => {});
return result;
}With this in place the warning is gone across our full app (verified with Environment: It would be great to see #29468 land — happy to test a dev release. |
| // pg.PoolClient does not support concurrent queries on the same connection, | ||
| // so we serialize all performIO calls with a mutex. | ||
| #mutex = new Mutex() |
There was a problem hiding this comment.
I think we should move the mutex to the base PgQueryable and do the serialization logic there instead of overriding performIO here.
As the comment says, pg.PoolClient doesn't allow this generally, doesn't matter if it's within a transaction or not. Batch transactions are just how you happen to be able to observe the issue right now in practice but the issue is more general.
|
Hey @matingathani & @tensordreams One data point supporting the approach: the same fix, per-client serialization with the pool unaffected, already shipped in the next-gen driver on main in #29839 |
A pg.PoolClient is a single connection and does not support concurrent queries (deprecated in pg@8, an error in pg@9), but the query interpreter loads sibling relations concurrently, so any query with 2+ relations inside a transaction triggered the deprecation warning. Serialize performIO in PgQueryable for single-connection clients, leaving the pool path parallel. Supersedes prisma#29468, fixes prisma#29407. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Camille Barneaud <1693643+gadcam@users.noreply.github.com>
Problem
When a
update(or any write operation) is executed withincluderelations, the query plan produces ajoinnode whose children are fetched viaPromise.all. Each child eventually callsqueryRawoncontext.queryable. Inside a transaction,context.queryableis aPgTransactionbacked by a singlepg.PoolClient— not a pool. Dispatching concurrentclient.query()calls on a single pg Client triggers:This becomes a hard error in
pg@9.0. Users are already hitting it in Prisma 7.5 (sincepg8.20.0 introduced the warning). The only current workaround is to downgradepgto 8.18.Reported in #29407.
Root cause
query-interpreter.ts,case 'join'(line ~243):Promise.allfires all child queries concurrently. Fine whencontext.queryableis aPool(each query gets its own connection), but not when it is aTransaction(single connection).Fix
Detect whether
context.queryableis aTransactionvia thecommitproperty (present on theTransactioninterface from@prisma/driver-adapter-utils). When inside a transaction, replacePromise.allwith a sequentialfor...ofloop. When using a pool the existing parallel behaviour is unchanged.Test plan
updatewith multipleincluderelations inside$transaction([...])no longer emits the pg deprecation warningupdatewith multipleincluderelations outside a transaction continues to fire relation queries in parallel (no performance regression)Fixes #29407