feat(postgres,sqlite): add typed temp table creation in transactions#836
feat(postgres,sqlite): add typed temp table creation in transactions#836paulwer wants to merge 1 commit 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:
📝 WalkthroughWalkthroughAdds a ChangesTransaction tempTable() API — Postgres and SQLite
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@packages/3-extensions/postgres/src/runtime/postgres.ts`:
- Line 225: Replace the bare `as unknown as TempTableJoinSource<Row>` cast with
`blindCast` in two locations: In
packages/3-extensions/postgres/src/runtime/postgres.ts at line 225 in the
`asJoinSource` method, replace `return source as unknown as
TempTableJoinSource<Row>` with `return blindCast<TempTableJoinSource<Row>,
'source implements TempTableJoinSource duck-type'>(source)`. Apply the identical
fix in packages/3-extensions/sqlite/src/runtime/sqlite.ts at line 194 in the
corresponding `asJoinSource` method. This ensures compliance with coding
guidelines that prohibit bare `as` casts in production code.
🪄 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
Run ID: 6a088fd8-abb9-4c95-af81-be3fa224b03b
📒 Files selected for processing (6)
packages/3-extensions/postgres/src/runtime/postgres.tspackages/3-extensions/postgres/test/postgres.test.tspackages/3-extensions/postgres/test/transaction.types.test-d.tspackages/3-extensions/sqlite/src/runtime/sqlite.tspackages/3-extensions/sqlite/test/transaction.test.tspackages/3-extensions/sqlite/test/transaction.types.test-d.ts
8cb7684 to
26257d6
Compare
aqrln
left a comment
aqrln
left a comment
There was a problem hiding this comment.
This is a good start, but, before discussing the implementation further, let's start with a spec that describes the user-facing API, and then add end-to-end tests.
| TempTableJoinSource<Row> & { | ||
| readonly name: string; | ||
| readonly fields: Row; | ||
| drop(): Promise<void>; |
There was a problem hiding this comment.
should we use Symbol.asyncDispose?
There was a problem hiding this comment.
Good call, implemented. The temp table handle now exposes Symbol.asyncDispose and maps it to drop(), so explicit cleanup and async disposal behave consistently.
I dont think, that would be widly used and we dont need it technical for cleanup the tempTable,because of the transactions itself, but supporting this pattern might help some folks.
| const tableName = resolveTempTableName(options); | ||
| const quotedTableName = quoteIdentifier(tableName); | ||
| const queryPlan = planFromAst(query.buildAst(), contract, 'dsl'); | ||
| const adapter = instantiateExecutionStack(stack).adapter; |
There was a problem hiding this comment.
this should not instantiate a new execution stack, we should already have access to everything here
There was a problem hiding this comment.
Agreed, fixed. I removed the extra stack instantiation path and now reuse the existing runtime/adapter context that is already available in the transaction flow. Please check
| }); | ||
|
|
||
| const createPlan = Object.freeze({ | ||
| sql: `CREATE TEMP TABLE ${quoteIdentifier(tableName)} AS ${lowered.sql}`, |
There was a problem hiding this comment.
we should use an AST node for this
There was a problem hiding this comment.
Done. I switched to an AST-backed path for temp table creation planning instead of constructing ad-hoc SQL execution outside the AST/plan flow.
| name: tableName, | ||
| fields: rowFields, | ||
| async drop(): Promise<void> { | ||
| await executeRawSql(txCtx, contract, `DROP TABLE IF EXISTS ${quotedTableName}`); |
There was a problem hiding this comment.
Applied the same AST-based approach.
|
|
||
| return { | ||
| async as<Row extends Record<string, ScopeField>>( | ||
| query: Subquery<Row>, |
There was a problem hiding this comment.
where does the subquery come from? could you add end-to-end tests that show how you use this to materialize an ORM or SQL query builder query into a temp table?
There was a problem hiding this comment.
Great point, added both variants in E2E. We now test materializing temp tables from SQL builder queries and from ORM queries using new internal method (please review as well or give me instructions, how else to solve, otherwise we could descide to omit ORM for now), and verify reuse in FROM and JOIN statements (in e2e).
dcb72e9 to
e2bb113
Compare
|
@aqrln I added a spec, please review and give me feedback |
e2bb113 to
00f7c16
Compare
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 `@packages/3-extensions/postgres/src/runtime/postgres.ts`:
- Around line 274-277: The getJoinOuterScope function contains a bare `as
Record<string, Row>` cast which violates coding guidelines that forbid bare `as`
casts in production code. Replace this bare cast by either creating a typed
local variable with explicit type annotation for the namespaces object or by
using a cast helper function like blindCast or castAs to safely convert the type
while maintaining code quality standards.
🪄 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
Run ID: 8c787472-2cd7-4cae-b28f-07224a662517
⛔ Files ignored due to path filters (1)
projects/temp-tables-in-transactions/spec.mdis excluded by!projects/**
📒 Files selected for processing (12)
packages/3-extensions/postgres/src/runtime/postgres.tspackages/3-extensions/postgres/test/postgres.test.tspackages/3-extensions/postgres/test/transaction.types.test-d.tspackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/exports/index.tspackages/3-extensions/sql-orm-client/src/internal-temp-table-source.tspackages/3-extensions/sql-orm-client/test/collection.as-subquery.test.tspackages/3-extensions/sqlite/src/runtime/sqlite.tspackages/3-extensions/sqlite/test/transaction.test.tspackages/3-extensions/sqlite/test/transaction.types.test-d.tstest/e2e/framework/test/sqlite/transaction.test.tstest/e2e/framework/test/transaction-orm.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/3-extensions/sql-orm-client/src/exports/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/3-extensions/sqlite/test/transaction.types.test-d.ts
- packages/3-extensions/postgres/test/transaction.types.test-d.ts
- packages/3-extensions/sqlite/test/transaction.test.ts
- packages/3-extensions/postgres/test/postgres.test.ts
- packages/3-extensions/sqlite/src/runtime/sqlite.ts
| getJoinOuterScope: () => ({ | ||
| topLevel: rowFields, | ||
| namespaces: { [alias]: rowFields } as Record<string, Row>, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace the new bare cast with a typed local (or cast helper).
This adds a bare as in production code. Use explicit typing (or blindCast/castAs) instead.
♻️ Proposed fix
- getJoinOuterScope: () => ({
- topLevel: rowFields,
- namespaces: { [alias]: rowFields } as Record<string, Row>,
- }),
+ getJoinOuterScope: () => {
+ const namespaces: Record<string, Row> = { [alias]: rowFields };
+ return {
+ topLevel: rowFields,
+ namespaces,
+ };
+ },As per coding guidelines, **/*.{ts,tsx,js,jsx} forbids bare as in production code and requires cast helpers or narrower typing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getJoinOuterScope: () => ({ | |
| topLevel: rowFields, | |
| namespaces: { [alias]: rowFields } as Record<string, Row>, | |
| }), | |
| getJoinOuterScope: () => { | |
| const namespaces: Record<string, Row> = { [alias]: rowFields }; | |
| return { | |
| topLevel: rowFields, | |
| namespaces, | |
| }; | |
| }, |
🤖 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-extensions/postgres/src/runtime/postgres.ts` around lines 274 -
277, The getJoinOuterScope function contains a bare `as Record<string, Row>`
cast which violates coding guidelines that forbid bare `as` casts in production
code. Replace this bare cast by either creating a typed local variable with
explicit type annotation for the namespaces object or by using a cast helper
function like blindCast or castAs to safely convert the type while maintaining
code quality standards.
Source: Coding guidelines
@prisma-next/extension-author-tools
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/middleware-cache
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/extension-postgis
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/extension-supabase
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/cli-telemetry
@prisma-next/config-loader
@prisma-next/emitter
@prisma-next/language-server
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
aqrln
left a comment
aqrln
left a comment
There was a problem hiding this comment.
Great, this is shaping up very nicely, I like the direction! Nice work 🙌
I left a bunch of comments in the spec, let's finalize the design and then update the implementation.
Also heads up that some jobs are failing on CI.
| .build(), | ||
| ).toArray(); | ||
|
|
||
| await temp.drop(); // or: await using temp = ... |
There was a problem hiding this comment.
We should be encouraging the use of the safer await using temp = ... pattern in the examples and docs.
This code snippet, as currently written, has a bug and does not always call drop(). It needs to use a try/finally block to do so consistently.
There was a problem hiding this comment.
(on the other hand, transaction rollback will also drop the table automatically, so 🤷)
| The builder exposes: | ||
|
|
||
| ```ts | ||
| as(query: TempTableQuerySource | OrmCollection): Promise<TempTableHandle<Row>> |
There was a problem hiding this comment.
This must not know anything about OrmCollection on this level. There should be a common interface that as() accepts and all query DSLs implement.
|
|
||
| ### Query sources accepted by `as(...)` | ||
|
|
||
| `as(...)` accepts two kinds of inputs: |
There was a problem hiding this comment.
It must accept infinite kinds of inputs that implement a common interface to get the AST. Query DSLs are pluggable and could even be implemented as third-party packages.
|
|
||
| ## Behavior | ||
|
|
||
| - Temp tables are scoped to the active transaction connection. They do not exist outside the transaction and cannot leak across requests or connection-pool members. |
There was a problem hiding this comment.
Do we actually guarantee that they are transaction scoped or session/connection scoped?
This influences the SQL statement we need to emit to create the temp table, and how important the drop API is.
There was a problem hiding this comment.
Oh and ON COMMIT DROP is a Postgres extension I think, so we can't actually guarantee transaction scope, unless we move the whole API into the target layer.
There was a problem hiding this comment.
@aqrln as mentioned below this works for postgres completly, but sqllite needs a cleanup process. I therefore introduced preCommitHooks and implemented a suiteable solution for that.
please review :)
| - `select(...)` on the collection limits the columns that appear in the temp table and in `handle.fields`. | ||
| - If no `select(...)` is provided, the ORM's default scalar projection is used. | ||
| - The normalisation is performed via an internal Symbol protocol (`INTERNAL_TO_TEMP_TABLE_QUERY_SOURCE`) on `Collection`. This is **not a public API** and is not exported as part of the ORM's user-facing surface. | ||
| - A public, standalone `asSubquery()` API on `Collection` is **out of scope for this PR** and is tracked as a follow-up if a general ORM-to-query-source conversion primitive is needed. |
There was a problem hiding this comment.
No, it's a blocker, the temp table API must not know anything about the ORM or the Connection class.
There was a problem hiding this comment.
@aqrln should there be a asSubquery handler, which returns the AST?
There was a problem hiding this comment.
Not sure yet about the name and whether it should return the raw AST, but let's do this for now
|
|
||
| - No `asRaw(...)` API — raw SQL strings are not accepted as a source; use the SQL builder's `raw\`...\`` tag to compose raw expressions into a structured query first. | ||
| - No temp-table alias method on the returned handle — the handle's `name` is the canonical table name and also serves as the default namespace alias in join field proxies. | ||
| - No support outside a transaction — the API is intentionally limited to transaction contexts; outside a transaction, different connection pool members may serve sequential queries, making a temp-table session guarantee impossible. |
There was a problem hiding this comment.
It is still session scoped by default even if you create it within a transaction (except the rollback case)
| ## Name semantics | ||
|
|
||
| - If `name` is omitted, a collision-resistant name is generated: `pn_temp_<20 hex chars>`. | ||
| - If provided, the name must satisfy `[A-Za-z_][A-Za-z0-9_]*` and must not exceed 63 characters (Postgres `NAMEDATALEN` limit; SQLite applies the same cap for consistency). |
There was a problem hiding this comment.
Should these rules be owned by the adapter, e.g. in AdapterProfile?
There was a problem hiding this comment.
@aqrln i mentioned it below already, but do you see any real benefit for the user to change the name used to create the temp table? in my opinion we could completly let the implementation descide the name.
by default a name currently is optional and can be user, otherwise the name is created to the according rule described in the adapter impl
There was a problem hiding this comment.
Agree. It's easy to add it later if a use case is found and someone requests it.
|
ON COMMIT DROP is postgres default and no extension if I understand it correctly, so for postgres we could enforce temp table only valid in execution context, if defined in statement. Its one of the base features and exists at least < PostgreSQL 7.x. Things I am thinking about which I wanted to share:
@aqrln any thoughts on those topics? |
0161372 to
1ff479e
Compare
|
@aqrln i have added an additional spec tempTable().from() + append() which enables users to "upload" data into temp tables for reuse in further calculations. please review :) Bulk import pipelines — materialise the incoming data rows once, then run validation queries, conflict detection, and inserts all against the temp table rather than re-parsing the input multiple times. Use const staging = await tx.tempTable({ name: 'staging' }).from([
{ name: 'id', type: 'int4' },
{ name: 'email', type: 'text' },
]);
await staging.append(importedRows); // load data
await staging.append(moreRows); // add more data if needed
// … run validations, conflict checks, final INSERT … |
It is also possible for users to obtain a pinned connection without a transaction. Those could make use of temporary tables too. |
1ff479e to
7331f81
Compare
@aqrln There wasnt a native withTransaction wrapper available yet, so i added it. Please check if thats realy wanted all in this PR or if thats out of scope tbh?! If thats not the aproach of waht you meant please iterate a bit more on it. |
022e813 to
4b189fa
Compare
|
Hey @paulwer, sorry for the delay! I think the feature is great and I'd like to get it finished and merged.
It's not, you need to opt in to it explicitly, see https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-PARMS-ON-COMMIT
Extension in the sense of Postgres-specific addition to SQL, not in the sense of loadable PostgreSQL extensions. It would still be a non-standard extension regardless of whether it's the default or not because it does not exist in standard SQL. Only Notably, even though Postgres doesn't use the non-standard If we don't allow custom temp table names and never reuse the same name twice, then I think the practical difference between So I think we should not allow custom/static names for transaction-scoped temp tables, and we should definitely have the cleanup API via We could have a different session-scoped temp table API, where customizing the name would make sense, but I don't think we need that right now.
Let's not do that right now. Session-scoped temp tables outside of transactions are okay for a pinned connection, but creating and dropping real tables accessible from multiple connections sounds like a recipe for race conditions. Those should probably be just normal tables defined in the contract and managed with migrations, even if the data is periodically cleaned. Or materialized views.
I'll have a look! |
|
@aqrln sounds lgtm, I will have a look in the next couple of days :)
Please also review my comment regarding Bulk import pipelines Thank you very much :) |
ad67ec6 to
bbd8207
Compare
Signed-off-by: paulwer <paul@wer-ner.de>
bbd8207 to
889dd8c
Compare
|
@aqrln I updated the branch based on the discussed points above |
2 participants
Signed-off-by: paulwer paul@wer-ner.de
Linked issue
https://discord.com/channels/937751382725886062/1514991788455104672
Summary
Adds a first-class, type-safe
tempTable()API to the transaction context of both the Postgres and SQLite runtime extensions.The API surface on the transaction context:
The builder exposes two creation modes:
as(query)— derives the table schema and initial data from any typed query source (SQL builder query or ORM collection). The returned handle is parameterised over the inferred row shape.from(columns)— creates an empty table with an explicitly specified column list. Data is loaded separately viaappend().The returned
TempTableHandle<Row>is a first-class join source (composable withinnerJoin,leftJoin, andFROM). It also exposes:append(input)— appends rows after creation; accepts either a typed query source or raw scalar rows ((string | number | boolean | null)[][]). Passing an empty array is a no-op.drop()/await using— explicit or automatic cleanup.nameandfields— resolved table name and typed column metadata.Cleanup strategy is target-specific: Postgres emits
ON COMMIT DROP; SQLite registers a pre-commit hook that issuesDROP TABLE IF EXISTSbefore theCOMMIT.Testing performed
pnpm --filter @prisma-next/postgres test— 86 passed (88 total; 2 pre-existing failures unrelated to this PR)pnpm --filter @prisma-next/sqlite test— 42/42 passedtest-d.ts), temp-table reuse in join composition,from()+append()round-trips, literal inlining (NULL, booleans, numbers, single-quote escaping), pre-commit hook cleanup, andawait usingdisposal.Skill update
n/a — no CLI commands, flags, config fields, or glossary terms were changed.
Checklist
git commit -s) per the DCO.TML-NNNN:convention.Notes for the reviewer
from()is columns-only by design — initial data is always loaded viaappend(). This keeps the creation step lightweight and makes multi-batch loading explicit.append()with a typed query source is type-checked againstRow; raw scalar rows are not (mapping SQL column types to TypeScript types at the API boundary is not feasible without a code-generation step).buildAst()always returnsTableSource.named(tableName), so the same handle can be passed to multiple joins in the same transaction without re-evaluating the source query.asRaw(string)API — raw SQL strings are not accepted as a source to preserve typed field-metadata for downstream join composition.