fix(qc): don't wrap single-statement plans in a transaction - #5840
Conversation
The query graph for updateMany and createMany is marked as requiring a transaction, so their plans were wrapped in a Transaction expression even though they translate to exactly one SQL statement. A single statement is atomic on its own, so the wrapper only added BEGIN/COMMIT round-trips — and made these operations fail on driver adapters without transaction support (e.g. Neon over HTTP), while the structurally identical deleteMany executed its single DELETE directly. Unwrap Transaction expressions in Expression::simplify when the inner expression contains at most one statement node. To let the new simplification see the wrapper, translate() now wraps the root before the simplify pass instead of after it. Fixes prisma/prisma#29748 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughThe query compiler now calculates an upper bound for database statement nodes across expression trees. Transaction expressions are simplified to 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 1
🤖 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 `@query-compiler/query-compiler/src/expression.rs`:
- Around line 262-267: Preserve transactions for chunkable Execute operations by
representing their statement count as potentially multiple or unbounded, or by
propagating explicit chunkability metadata. In
query-compiler/query-compiler/src/expression.rs lines 262-267, update
max_statement_count for Execute; in lines 209-220, remove the transaction only
when the execution-level bound—not merely the AST-node count—proves exactly one
statement.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a054c1ce-8e58-4ab3-931c-c36eee4342dd
⛔ Files ignored due to path filters (3)
query-compiler/query-compiler/tests/snapshots/queries__queries@create-many-and-return.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@create-many.json.snapis excluded by!**/*.snapquery-compiler/query-compiler/tests/snapshots/queries__queries@update-many.json.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
query-compiler/query-compiler/src/expression.rsquery-compiler/query-compiler/src/translate.rsquery-compiler/query-compiler/tests/data/update-many.json
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merging this PR will not alter performance
Comparing Footnotes
|
|
Sorry for the broken CI @gordyf, it looks like some recent changes in the main repo might have broken it. I'll have a look. |
…29771) Part of #29748. Implements the query interpreter side of [the approach agreed in prisma/prisma-engines#5840](prisma/prisma-engines#5840 (comment)): the query compiler stops wrapping plans in a transaction just because they contain a chunkable statement, and instead the interpreter starts a transaction at runtime iff the statement was actually split into multiple queries. ## Problem Whether a chunkable statement splits cannot be decided at plan compilation time: it depends on the number of bound values (hidden behind a placeholder) and on the adapter's `maxBindValues`, both only known at runtime. Today the chunks of a split statement run without any transaction for `deleteMany` and reads, which: - leaves earlier chunks committed if a later chunk fails (partial write), and - lets concurrent readers observe a half-applied `deleteMany`, and violates repeatable-read semantics for chunked reads on e.g. MySQL. Once prisma/prisma-engines#5840 lands, single-statement `createMany`/`updateMany` plans lose their compile-time `transaction` wrapper too (so they work on adapters without transaction support), which extends the same gap to their chunked executions. ## Solution `QueryInterpreter` now wraps the rendered queries of a `query` or `execute` node in an internal transaction when: - rendering produced **more than one** chunk, and - the plan is not already executing inside a transaction — neither an interactive transaction (`transactionManager: { enabled: false }`) nor an enclosing `transaction` plan node (tracked via a new `insideTransaction` context flag). Single-chunk statements keep executing directly on the current queryable, so the common case is unchanged and keeps working on adapters without transaction support (e.g. Neon HTTP). A multi-chunk statement on such an adapter will now fail up front instead of applying a partial, non-atomic write. This fix stands on its own (it fixes chunked `deleteMany` and chunked reads on current `main`) and complements prisma/prisma-engines#5840 without depending on it. ## Tests Added unit tests in `query-interpreter.test.ts` covering: chunked `execute` and `query` run inside a transaction and commit; rollback when a later chunk fails; no transaction for single-chunk statements; no nested transaction when inside a `transaction` plan node or an interactive transaction. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Alexey Orlenko's AI Agent <robot@aqrln.net>
Fixes prisma/prisma#29748. Supersedes prisma/prisma#29759, following the feedback there that this belongs in
Expression::simplifyrather than in the query interpreter.Problem
The query graphs for
updateManyandcreateManyreportneeds_transaction() == true, so their plans get wrapped in aTransactionexpression even though they translate to exactly one SQL statement. A single statement is atomic on its own, so the wrapper only addsBEGIN/COMMITround-trips — and it makes both operations fail hard on driver adapters without transaction support (e.g.PrismaNeonHttp), while the structurally identicaldeleteManyexecutes its singleDELETEdirectly:Solution
Expression::simplifynow unwrapsTransaction(expr)whenexprcontains at most one statement node (Query/Execute). The newmax_statement_counthelper computes an upper bound over the expression tree, taking the maximum over mutually exclusiveIfbranches and treating nestedTransactionexpressions as unbounded.translate()wraps the root inTransactionbefore the simplify pass (previously after), so the new simplification can see the wrapper. Plans with two or more statements keep the transaction exactly as before.Note on chunking: a single chunkable statement (e.g. a large
createManyexceeding the bind-parameter limit) may still be split into multiple queries by the client at runtime, and those chunks now run without a transaction. This matches how chunkeddeleteManyand read queries — which are never wrapped in a transaction — behave today; a comment insimplifydocuments this.Tests
update-manycase to the query translation snapshot tests (there was none).create-manyandcreate-many-and-returnsnapshots: thetransactionwrapper around their single statement is gone; multi-statement snapshots (nested writes, m2m, etc.) are unchanged.prisma/prisma(unmodifiedmain), and running a generated client against a driver adapter whosestartTransactionrejects:createMany/updateManynow succeed, and explicit$transaction([...])still fails with the adapter's error as documented.🤖 Generated with Claude Code