sql: expose column type metadata on query results - #30037
Conversation
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
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:
WalkthroughThis change adds column metadata exposure to SQL query results. Type definitions for Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/sql/sql.test.ts`:
- Around line 2998-3003: Add coverage for the zero-field RowDescription emitted
by a bare `SELECT;` in a multi-statement simple query by extending the test for
sql.simple() to include a `select 1 as a; SELECT; select 'x'::text as b, 2::int4
as c` sequence and assert that the middle result has zero columns (empty columns
array) while surrounding results keep their columns/types; additionally, ensure
the implementation that processes RowDescription messages unconditionally resets
the parser state variables `cached_structure`, `needs_duplicate_check`, and
`fields_flags` on every RowDescription handling path (the code that updates
these flags for simple() result sets) so a zero-field RowDescription does not
leave stale cached structure for the next result set.
- Around line 2956-3058: Add tests in the "result.columns / result.statement"
suite that assert .columns and .statement are populated even for zero-row result
sets (e.g. using sql`select 1 as x LIMIT 0` or sql`select 1 as x where false`)
so schema metadata is attached independent of row decoding; specifically call
sql`...` to get a Result, then assert result.columns contains the expected
column descriptors (name and type) and result.statement.string/columns behaves
the same as for non-empty results, ensuring code paths that currently attach
metadata only when rows are decoded are exercised and fixed.
In `@test/regression/issue/26809.test.ts`:
- Around line 64-69: Add a regression case to the existing test "multi-statement
simple() has per-result columns" to cover a zero-column result followed by a
normal result: call sql`SELECT; SELECT 'x'::text as b, 2::int4 as c`.simple()
and assert that results[0].columns maps to an empty array (no names),
results[1].columns.map(c => c.name) equals ["b","c"], and
results[1].columns.map(c => c.type) equals [25,23]; update the test body in the
same test block so the zero-field RowDescription path is exercised before the
next result set.
- Around line 6-11: The current top-level import of dockerCompose causes side
effects on non-Docker runs; change getPostgresURL to lazy-load the module only
when Docker is enabled by replacing the static import usage with a dynamic
import inside the isDockerEnabled() branch (e.g., const dockerCompose = await
import("../../docker/index.ts")) and then call
dockerCompose.ensure("postgres_plain") to obtain info and build the URL; this
keeps the module from being evaluated on non-Docker runs and preserves use of
dockerCompose.ensure.
🪄 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.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56f3cd53-9da0-40a7-8fef-23fb39fa7d1e
📒 Files selected for processing (12)
packages/bun-types/sql.d.tssrc/js/internal/sql/mysql.tssrc/js/internal/sql/postgres.tssrc/js/internal/sql/shared.tssrc/sql/mysql/MySQLQuery.zigsrc/sql/mysql/js/JSMySQLQuery.zigsrc/sql/mysql/protocol/ColumnDefinition41.zigsrc/sql/postgres/PostgresSQLQuery.zigsrc/sql/postgres/protocol/FieldDescription.zigtest/js/sql/sql-mysql.test.tstest/js/sql/sql.test.tstest/regression/issue/26809.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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)
src/sql/postgres/protocol/FieldDescription.zig (1)
41-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd cleanup for
field_nameon decode failure paths.
ColumnIdentifier.init(name)can allocate, but later fallible reads (orData.create) can fail beforethis.*assignment, leavingfield_nameunfreed on error.♻️ Proposed fix
- const field_name = try ColumnIdentifier.init(name); + var field_name = try ColumnIdentifier.init(name); + errdefer field_name.deinit();As per coding guidelines, "In Zig code, be careful with allocators and use defer for cleanup".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sql/postgres/protocol/FieldDescription.zig` around lines 41 - 72, Create a scoped cleanup for the ColumnIdentifier allocation to avoid leaks on fallible reads: after calling ColumnIdentifier.init(name) store it into an optional (e.g., var field_name_opt: ?ColumnIdentifier = try ColumnIdentifier.init(name)) and add a defer that deinitializes it (defer if (field_name_opt) |fn| fn.deinit();). Continue using reader.* and Data.create as before, and just before assigning this.* clear ownership by setting field_name_opt = null and move the value into this.name_or_index (or otherwise transfer ownership) so the defer won’t free it on success; this ensures field_name is freed on any error path but retained on success.
🤖 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 `@src/sql/postgres/protocol/FieldDescription.zig`:
- Around line 41-72: Create a scoped cleanup for the ColumnIdentifier allocation
to avoid leaks on fallible reads: after calling ColumnIdentifier.init(name)
store it into an optional (e.g., var field_name_opt: ?ColumnIdentifier = try
ColumnIdentifier.init(name)) and add a defer that deinitializes it (defer if
(field_name_opt) |fn| fn.deinit();). Continue using reader.* and Data.create as
before, and just before assigning this.* clear ownership by setting
field_name_opt = null and move the value into this.name_or_index (or otherwise
transfer ownership) so the defer won’t free it on success; this ensures
field_name is freed on any error path but retained on success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8cd35c16-edcb-46a3-b3e0-c17650bd1899
📒 Files selected for processing (2)
src/sql/mysql/protocol/ColumnDefinition41.zigsrc/sql/postgres/protocol/FieldDescription.zig
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sql/postgres/PostgresSQLConnection.zig`:
- Around line 1582-1594: The current guard "if (statement.fields.len > 0)"
prevents resetting per-result caches for zero-field RowDescriptions; modify the
logic in the request.flags.simple branch so that freeing/deinit of each field
and bun.default_allocator.free(statement.fields) remain conditional on
statement.fields.len > 0, but always execute
statement.cached_structure.deinit(), set statement.cached_structure = .{}, set
statement.needs_duplicate_check = true, and set statement.fields_flags = .{}
unconditionally (i.e., move those four cache-reset actions out of the fields.len
> 0 block while keeping field deinit/free inside it).
In `@test/js/sql/postgres-result-columns.test.ts`:
- Around line 120-141: Update the "result.columns is populated even for zero-row
result sets" test to also assert that the statement metadata references the same
columns array: after the existing expects for result length and result.columns,
add an assertion that result.statement.columns is exactly the same array as
result.columns (e.g., expect(result.statement.columns).toBe(result.columns)) so
the zero-row invariant is covered; locate this in the test function where result
is obtained from sql`select id, msg from t where false`.simple().
In `@test/js/sql/sql-mysql.test.ts`:
- Around line 79-136: Add a zero-row metadata test to cover the no-row path: run
a query like sql`SELECT CAST(1 AS SIGNED) AS x WHERE FALSE` (or use LIMIT 0) and
assert that result.columns exists, result.columns.map(c => c.name) equals ["x"],
and result.statement.columns === result.columns; add the case alongside the
other column tests (reusing the same sql`...`.values()/.simple() variants as
appropriate) so the code paths in result.columns and Result.statement.columns
for zero-row results are exercised.
🪄 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.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d85a2f3-ba8c-419b-a0d1-182993e72a96
📒 Files selected for processing (6)
src/sql/mysql/protocol/ColumnDefinition41.zigsrc/sql/postgres/PostgresSQLConnection.zigsrc/sql/postgres/protocol/FieldDescription.zigtest/js/sql/postgres-result-columns.test.tstest/js/sql/sql-mysql.test.tstest/js/sql/sql.test.ts
There was a problem hiding this comment.
No remaining issues from my side, but this adds new public API surface (.columns/.statement) and touches memory-lifetime-sensitive Zig in both SQL drivers — worth a maintainer pass on the API shape and the Data ownership changes before merging.
Extended reasoning...
Overview
This PR adds result.columns and result.statement to Bun.sql query results, exposing wire-protocol column type metadata (Postgres OIDs, MySQL column-type codes). It spans 13 files: new public TypeScript types in packages/bun-types/sql.d.ts, JS-side resolve-callback plumbing in src/js/internal/sql/{shared,postgres,mysql}.ts, and native Zig changes in both drivers — buildStatementJS() in PostgresSQLQuery.zig / JSMySQLQuery.zig, owned-string lifetime changes in FieldDescription.zig / ColumnDefinition41.zig, and a per-command statement.fields reset in PostgresSQLConnection.zig's .CommandComplete handler. Test coverage includes a new mock-Postgres-server suite, real-Postgres tests in sql.test.ts, and MySQL tests in sql-mysql.test.ts.
Security risks
None identified. The change is read-only metadata exposure derived from server-sent RowDescription / ColumnDefinition41 packets. No new user input is parsed or interpolated into queries; statement.string echoes the already-constructed query text. No auth, crypto, or permission surfaces are touched.
Level of scrutiny
High. This is a new public API whose shape (field names, Postgres vs MySQL divergence, postgres.js compatibility) is a design decision a maintainer should sign off on. More importantly, the native-side changes alter Data ownership semantics on the hot path of every SQL query: over the course of this PR's review, five distinct memory-safety / lifetime issues were found and fixed (UAF on .temporary slices, double-free via aliased .owned ByteList, leak on prepared-statement re-execution, stale fields on multi-statement non-SELECTs, and an errdefer gap). All are now resolved and the latest bug-hunting pass found nothing, but the iteration count signals this code is subtle enough to merit human review.
Other factors
All prior inline comments (mine and CodeRabbit's) are resolved. Test coverage is thorough, including a Docker-free mock-server suite plus ASAN-targeted tests for the >15-byte-name ownership path. CI build #49703 shows musl build failures and two unrelated-looking Windows test failures (astro-post.test.js segfault, hot.test.ts); the musl failures should be confirmed as infra rather than code before merge. buildStatementJS runs unconditionally on every query resolve, so a maintainer may also want to weigh the per-query allocation cost.
9d1cfaa to
0040200
Compare
68e5c98 to
03dcba0
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/bun.js/bindings/GeneratedBindings.zig:1-6— These two files (andGeneratedJS2Native.zignext to it) are stale codegen artifacts at the legacy pre-restructuresrc/bun.js/bindings/path that were accidentally committed —.gitignoreonly covers the newsrc/jsc/bindings/location, so git picked them up. They're ~780 lines of generated code completely unrelated to the SQL column-metadata feature, reference import paths that no longer exist, and are unreferenced by the build; please remove both files (and the now-emptysrc/bun.js/bindings/+src/bun.js/directories) from the PR.Extended reasoning...
What the issue is
This PR adds two new files at
src/bun.js/bindings/GeneratedBindings.zig(391 lines) andsrc/bun.js/bindings/GeneratedJS2Native.zig(388 lines). Both file headers state they are generated bysrc/codegen/generate-js2native.ts, and CLAUDE.md's "Generated code" section lists these as build artifacts that should never be committed. They are entirely unrelated to the SQL column-metadata feature this PR implements.How they slipped in
The repo restructure (commit
c8b4c36) movedsrc/bun.js/bindings/→src/jsc/bindings/and updated.gitignoreaccordingly: lines 128-129 now ignoresrc/jsc/bindings/GeneratedJS2Native.zigandsrc/jsc/bindings/GeneratedBindings.zig(the new path), but the oldsrc/bun.js/bindings/path is no longer covered. A stale codegen run on this branch (likely during the68e5c98interface-sync commit) wrote output to the legacy location, and since.gitignoreno longer matches it,git addswept the files into the commit.Why these files are dead
src/bun.js/now contains only abindings/subdirectory containing only these two files — everything else undersrc/bun.js/was moved out in the restructure.src/bun.zigimports the generated bindings from./jsc/bindings/(the new path), not./bun.js/bindings/. Grepping the whole repo forbun.js/bindings/Generatedreturns zero references — nothing in the build system or any source file imports these.- The committed files contain stale relative imports like
@import("../../bake.zig"),@import("../../crash_handler.zig"),@import("../../sql/mysql.zig"),@import("../../sql/postgres.zig")— none of which exist post-restructure (they are now directories, e.g.src/bake/,src/crash_handler/). If anything did import these files they would fail to compile. - Zig only compiles what is reachable from the root module, so the build is unaffected.
Step-by-step proof
- PR diff shows both files as brand-new (
--- /dev/null), totalling 779 added lines. ls -la src/bun.js/→ onlybindings/.ls src/bun.js/bindings/→ onlyGeneratedBindings.zigandGeneratedJS2Native.zig..gitignore:128-129→src/jsc/bindings/GeneratedJS2Native.zigandsrc/jsc/bindings/GeneratedBindings.zig. No entry forsrc/bun.js/bindings/.src/jsc/bindings/exists with hundreds of files — it is the actual current bindings location.GeneratedJS2Native.zig:1→//! This file is generated by src/codegen/generate-js2native.ts.rg 'bun.js/bindings/Generated'across the repo → 0 hits.
Impact
No runtime or build impact (the files are unreferenced and Zig's lazy compilation ignores them), but it's 779 lines of accidentally-committed pollution that bloats the diff, obscures review, and would land dead generated code in the tree.
Fix
git rm src/bun.js/bindings/GeneratedBindings.zig src/bun.js/bindings/GeneratedJS2Native.zigand remove the now-emptysrc/bun.js/bindings/andsrc/bun.js/directories. Optionally addsrc/bun.js/bindings/GeneratedBindings.zigandsrc/bun.js/bindings/GeneratedJS2Native.zigto.gitignoreso other in-flight branches don't hit the same trap.
There was a problem hiding this comment.
No issues found in the latest revision, but this introduces new public API (result.columns / result.statement shapes for PG and MySQL) and changes memory ownership in the Zig protocol decoders, so it's worth a human pass on the API design and the ColumnDefinition41 / FieldDescription / CommandComplete lifecycle changes.
Extended reasoning...
Overview
This PR adds .columns and .statement metadata to Bun.sql query results across PostgreSQL and MySQL, spanning 13 files: public type definitions (sql.d.ts), internal JS resolve callbacks (postgres.ts, mysql.ts, shared.ts), Zig wire-protocol decoders (FieldDescription.zig, ColumnDefinition41.zig), the query/connection layer (PostgresSQLQuery.zig, PostgresSQLConnection.zig, JSMySQLQuery.zig, MySQLQuery.zig), and three test files.
Security risks
None identified. The change is read-only metadata exposure derived from server-sent RowDescription / ColumnDefinition41 packets; no new user input is parsed, no auth/crypto/permissions paths are touched.
Level of scrutiny
High. This is not a mechanical change:
- It defines new public API surface (
Bun.SQL.ResultColumn/ResultStatement) with deliberately different shapes per adapter (PG:{name, type, table, number}; MySQL:{name, type, table, length, flags}). That's a design decision a maintainer should sign off on. - It changes memory ownership in hot protocol decoders:
ColumnDefinition41.decodeInternalandFieldDescription.decodeInternalnow heap-ownname/tableinstead of holding.temporaryrecv-buffer slices, with newdeinit()-before-overwrite calls for the prepared-statement re-decode path. - It adds a field-reset block to the Postgres
CommandCompletehandler for simple-mode multi-statement queries, which alters per-result-set state lifecycle.
Other factors
The PR went through several rounds of memory-safety fixes flagged during review (UAF on temporary slices, a double-free from Data.toOwned() aliasing, a leak on re-decode, a missing errdefer), all of which are now addressed and covered by tests including ASAN-targeted cases. The current bug-hunting pass found nothing. Test coverage is thorough (mock-server wire-protocol tests + real-DB tests for both adapters). The one CI failure (test-http-should-emit-close-when-connection-is-aborted on Windows) is unrelated. Given the API-design and Zig memory-lifecycle aspects, this exceeds the threshold for auto-approval.
03dcba0 to
1ec27b0
Compare
There was a problem hiding this comment.
No further issues found — all earlier memory-safety findings have been addressed and the Rust port mirrors the Zig changes. Deferring to a human for sign-off since this introduces a new public API surface (result.columns / result.statement) and adds per-query native allocation in both the Zig and Rust SQL drivers.
Extended reasoning...
Overview
This PR adds .columns and .statement metadata to Bun.sql query results, exposing wire-protocol column type information (PostgreSQL OIDs, MySQL type codes) so callers can distinguish e.g. jsonb from text[]. It spans 19 files: public type declarations (sql.d.ts), the JS resolve-callback plumbing (shared.ts, postgres.ts, mysql.ts), native Zig protocol/decode and query-result paths for both Postgres and MySQL, the parallel Rust implementations of the same, and three test files including a new mock-Postgres-server suite.
Security risks
None identified. The change is read-only metadata exposure derived from server-sent RowDescription/ColumnDefinition packets; no new user input reaches a parser or allocator. The strings surfaced to JS are copied via Data.create / createUTF8ForJS rather than aliased from the socket buffer.
Level of scrutiny
High. This is not a mechanical change:
- It defines a new public API shape (
Bun.SQL.ResultColumn/ResultStatement) that downstream tooling (Prisma adapters etc.) will depend on, so the field naming and per-driver semantics deserve a maintainer's eye. - It touches memory-sensitive native code in the hot path of every query resolve. Over the course of review, four separate memory-management bugs were found and fixed (UAF on
.temporaryslices, double-free viaData.toOwned()aliasing for >15-byte names, per-execution leak on prepared-statement re-decode, and stalestatement.fieldscarryover in multi-statement.simple()). All are now addressed with tests, but the density of issues during iteration suggests a human pass over the finalColumnDefinition41.zig/FieldDescription.zig/PostgresSQLConnection.zigownership model is worthwhile. buildStatementJSnow runs unconditionally on every query resolve (Postgres and MySQL), allocating a JS array + N objects per result set even when the caller never reads.columns. That's a deliberate trade-off vs. the lazy-getter approach in the superseded #18892, and a maintainer should confirm it's acceptable.- The change is duplicated across Zig and Rust implementations; the Rust port (commit 1ec27b0) was added late and mirrors the Zig logic but hasn't had the same iterative scrutiny.
Other factors
Test coverage is solid: real-Postgres (sql.test.ts), real-MySQL (sql-mysql.test.ts), and a Docker-free mock-server suite (postgres-result-columns.test.ts) covering zero-row results, duplicate names, multi-statement .simple(), signed attnums, and the >15-byte-name ownership case under GC. The two CI failures in build #55988 (node-http-backpressure, jsc-stress) are unrelated to this change. All review threads are resolved.
c438393 to
0dc95d7
Compare
There was a problem hiding this comment.
No outstanding issues from my side — all earlier findings have been addressed. Deferring to a human reviewer since this introduces a new public Bun.SQL API surface across the native Postgres/MySQL drivers and touches packages/bun-types/ (CODEOWNER'd).
Extended reasoning...
Overview
This PR adds .columns and .statement metadata to Bun.sql query results, exposing wire-protocol column type information (PostgreSQL OIDs, MySQL type codes) so driver adapters can distinguish e.g. jsonb from text[]. It spans 19 files: public type definitions (packages/bun-types/sql.d.ts), the internal JS glue (src/js/internal/sql/{shared,postgres,mysql}.ts), native protocol decoders in both Zig and Rust (FieldDescription, ColumnDefinition41), the JSC query bridges (PostgresSQLQuery, JSMySQLQuery, PostgresSQLConnection), and three test files including a new mock-Postgres-server suite.
Security risks
None identified. The change is read-only metadata exposure derived from server-sent RowDescription/ColumnDefinition packets; no new user input is parsed, no auth/crypto/permissions paths are touched, and no new network surface is opened. The memory-lifetime concerns (UAF on .temporary slices, double-free via Data.toOwned() aliasing, leak on prepared-statement re-decode) were all raised during review and fixed in commits 09bb472 / 3319dba / 9d1cfaa with ASAN-verified tests.
Level of scrutiny
High — this warrants human review rather than shadow-approval:
- New public API design:
Bun.SQL.ResultColumn/ResultStatementshapes (PostgreSQL{name, type, table, number}vs MySQL{name, type, table, length, flags}) are being locked in for postgres.js compatibility; a maintainer should sign off on the surface. - CODEOWNER'd path:
packages/bun-types/sql.d.tsfalls under thepackages/bun-types/and*.d.tsCODEOWNERS entries. - Native memory management: changes to
Dataownership in hot per-query decode paths across two languages (Zig + Rust ports), plus a new per-CommandCompletefield-reset in the Postgres simple-query state machine. - Per-query overhead:
buildStatementJSnow runs unconditionally on every result, allocating a JS array + N objects + 2N strings per query even when.columnsis never read.
Other factors
The PR has been through several review iterations; every inline finding I raised (status-transition ordering, .temporary UAF, .owned aliasing double-free, re-decode leak, stale-fields inheritance in .simple(), signed i16 attnum, interface-type drift) has been addressed and resolved with targeted tests. The current bug-hunt pass found nothing new. Test coverage is thorough (real-Postgres, real-MySQL, and a Docker-free mock-server suite). The one acknowledged follow-up — wiring ResultColumn[] into a public SQLResult<T> type — is reasonably out of scope here.
9c349a3 to
9021987
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/bun_core/debug.rs:1-4— This file is a 387-line frame-pointer stack unwinder for the crash handler — entirely unrelated to SQL column metadata — and appears to have been accidentally staged into commit 9c349a3. It is also dead code:src/bun_core/lib.rs:3368already definespub mod debug { … }as an inline module, so Rust never looks fordebug.rsand the file is not compiled. Please drop it from this PR and submit it with the crash-handler work it belongs to.Extended reasoning...
What this is
src/bun_core/debug.rsis a brand-new 387-line file implementing a frame-pointer stack unwinder:SourceLocation,SymbolInfo,MemoryAccessor,StackIterator,frame_address(),capture_current(),capture_from_context(). Per its own header comment it is a port ofstd.debugfor "the crash handler,StoredTrace, andbtjs". This PR is about exposing SQL column type metadata on query results — nothing in the PR title, description, CodeRabbit walkthrough, or any of the 22 other changed files references stack unwinding or crash handling.Why it's accidentally committed, not intentional
git log --diff-filter=A -- src/bun_core/debug.rsshows it was added in commit 9c349a3, whose message is "mysql: invalidate cached statement metadata when column definitions change" — a commit otherwise consisting entirely of changes toColumnDefinition41.rs,MySQLConnection.rs,MySQLStatement.rs,JSMySQLQuery.rs, andsql-mysql.test.ts. The commit message makes zero mention of debug/stack-trace work. The file does not exist onorigin/main. CodeRabbit's "Out of Scope Changes" check passed earlier in the PR, but ran before this commit was pushed (the check timestamp predates 9c349a3). This has all the hallmarks of a straygit addfrom an unrelated work-in-progress branch.Why it's dead code
src/bun_core/lib.rsis not in this PR's changed files. At line 3368 it already contains:pub mod debug { /// Zig: `std.debug.SourceLocation`. #[derive(Clone)] pub struct SourceLocation { ...
This is an inline module (
pub mod debug { … }with a body), not a file-module declaration (pub mod debug;). Per Rust's module rules, when a module is defined inline, the compiler does not look for a correspondingdebug.rsfile — the inline body is the module. So the newsrc/bun_core/debug.rsis never compiled at all; it's an orphaned file in the source tree. The comment atlib.rs:3367("pending a dedicatedbun_debugcrate") confirms the new file is an in-progress expansion of that inline module that hasn't been wired up yet.Step-by-step proof
git log --diff-filter=A -- src/bun_core/debug.rs→9c349a3b mysql: invalidate cached statement metadata when column definitions change. The file was added in a commit whose subject and body are exclusively about MySQL statement metadata.grep -n 'pub mod debug' src/bun_core/lib.rs→3368:pub mod debug {(note the opening brace — inline body follows).src/bun_core/lib.rsis absent from the PR's 23 changed files, so it was not changed topub mod debug;to point at the new file.- Therefore
rustccompilesbun_core::debugfrom the inline body atlib.rs:3368-…and never readssrc/bun_core/debug.rs. The file contributes zero bytes to the build. - Nothing in the SQL changes (
src/sql/**,src/sql_jsc/**,src/js/internal/sql/**) referencesStackIterator,MemoryAccessor,capture_current, or anything else from this file.
Impact
No runtime or build impact (the file is uncompiled), but merging it would:
- Land 387 lines of unrelated, unreviewed crash-handler work under a misleading commit message, making
git blame/git logarchaeology harder. - Create a confusing situation where
src/bun_core/debug.rsexists butbun_core::debugresolves to different code inlib.rs. - Bypass review of the actual feature (the stack unwinder) by smuggling it in with an unrelated PR.
Fix
git rm src/bun_core/debug.rsand amend/rebase it out of this PR. Submit it separately with the crash-handler /bun_debug-crate work it belongs to (wherelib.rswould presumably be changed topub mod debug;to actually use it).
There was a problem hiding this comment.
No issues found in the latest revision — all earlier review feedback has been addressed. That said, this introduces a new public API surface (result.columns / result.statement) and touches lifetime-sensitive native code across both the Zig and Rust SQL drivers, so it's worth a maintainer sign-off on the API shape and the cache-invalidation / ownership changes before merging.
Extended reasoning...
Overview
This PR adds .columns and .statement metadata to Bun.sql query results, exposing wire-protocol column type information (PostgreSQL OIDs, MySQL column-type codes) so driver adapters like Prisma can distinguish e.g. jsonb from text[]. It spans 22 files: public type definitions (sql.d.ts), JS-side resolve-callback plumbing (shared.ts, postgres.ts, mysql.ts), native protocol-layer changes in both Zig and Rust (FieldDescription, ColumnDefinition41, PostgresSQLQuery, JSMySQLQuery, PostgresSQLConnection, MySQLConnection, statement structs), and three test files including a new mock-Postgres-server suite.
Security risks
None identified. The change surfaces metadata the server already sends in RowDescription / ColumnDefinition41 packets; there's no new input parsing, auth, or privilege surface. The data flows server→client only.
Level of scrutiny
High. This is a new public API (the ResultColumn / ResultStatement shape becomes part of Bun's stable surface), and the implementation threads through memory-ownership-sensitive native code in two parallel implementations (Zig + Rust). Over the PR's lifetime the bug-hunting system found and the author fixed: a UAF from .temporary slices outliving the read buffer, a double-free from Data.toOwned() aliasing, a per-execution leak on prepared-statement re-decode, stale-column inheritance across simple-protocol CommandComplete, stale cached metadata for equal-width multi-result-sets, and a signed/unsigned attnum mismatch. All are resolved with regression tests, but the density of lifetime issues found during review is itself a signal that the final state deserves a careful human pass — particularly the new cached_statement_js Strong-reference cache and its invalidation sites, and the ColumnDefinition41::decode change-detection return value.
Other factors
- Test coverage is thorough: a mock-wire-protocol suite that runs without Docker, plus Docker-gated real-DB tests for both Postgres and MySQL covering zero-row results, duplicate names, multi-statement
.simple(), long (>15-byte) column-name ownership, and statement-object reuse. - The robobun CI comment lists build failures, but they appear to be pre-existing/unrelated (freebsd
unused label, muslbuild-cpp,-no-pielinker warnings) rather than failures in the changed code. - API-design questions a maintainer may want to weigh in on: the per-dialect
ResultColumnshape (Postgres usestable: number, MySQL usestable: string; Postgres hasnumber, MySQL haslength/flags), and whether the{ string, columns }object should be cached/shared across executions vs. fresh per query (this PR caches, matching postgres.js semantics).
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
There was a problem hiding this comment.
No issues found in this pass. Given this introduces a new public API surface (result.columns / result.statement) and touches native statement caching / memory ownership across both SQL drivers, it's worth a human sign-off before merge.
Extended reasoning...
Overview
This PR adds .columns and .statement metadata to Bun.sql query results, exposing wire-protocol column type information (PostgreSQL OIDs / MySQL type codes) so driver adapters can distinguish e.g. jsonb from text[]. It spans 18 files: public type definitions (sql.d.ts), prose docs (sql.mdx), JS-side result plumbing (shared.ts/postgres.ts/mysql.ts), native Rust changes to both drivers' protocol decoding, query/statement objects, and connection state machines (FieldDescription.rs, ColumnDefinition41.rs, PostgresSQLQuery.rs, JSMySQLQuery.rs, PostgresSQLConnection.rs, MySQLConnection.rs, *Statement.rs), plus a new mock-server test file and additions to the docker-gated suites.
Security risks
None identified. The change is read-only metadata exposure derived from data the server already sends; no new user input is parsed, no auth/crypto/permissions paths are touched, and the SQL text in statement.string is the already-parameterized string the client itself constructed.
Level of scrutiny
High. This is not a mechanical change:
- It defines a new, user-facing, stable API surface (shape of
ResultColumn/ResultStatement) that adapters like Prisma will depend on. - It adds a
StrongOptionalJSC-reference cache (cached_statement_js) on native statement objects with non-trivial invalidation rules tied to wire-protocol re-decode paths in both drivers. - It changes ownership semantics of
Datafields inColumnDefinition41/FieldDescription(temporary→owned copies) on hot decode paths. - Earlier review rounds on this PR surfaced and fixed several memory-safety issues (UAF on temporary read-buffer slices, double-free via aliased
Data::Owned, per-execution leaks on re-decode, stale-cache correctness for equal-width multi-result-sets). All are resolved and now have regression tests, but the density of such findings indicates the change sits in tricky territory.
Other factors
All 17 prior inline review threads (mine and CodeRabbit's) are resolved; the bug-hunting pass on the current revision found nothing. Test coverage is thorough — a Docker-free mock-Postgres-server suite plus docker-gated PostgreSQL/MySQL/MariaDB tests covering zero-row, duplicate-name, multi-statement .simple(), stale-column inheritance, long-name ownership, and prepared-statement metadata reuse. The PR description documents that the per-statement caching keeps the issue-28632 RSS regression test within its ASAN threshold. CI is building on the latest rebase. Net: the PR looks correct and well-tested, but the scope (new public API + native lifetime/caching across two drivers) is beyond what I'd auto-approve.
|
@robobun One of buns strengths is performance and throughput, wouldn't this be a performance hit? Should attaching this information be opt in instead? |
Adds result.columns and result.statement to Bun.sql query results for PostgreSQL (pg_type OIDs) and MySQL (column-type codes), with per-statement caching of the statement JS object and invalidation when column definitions change.
46ac248 to
31e779f
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/sql/mysql/protocol/ColumnDefinition41.rs:121-143— The comment-cop bot has 14 unresolved flags on this PR, and they're right: several new comment blocks exceed REVIEW.md's "one line" rule and duplicate each other — most visibly here, where two consecutive blocks (~21 lines for ~5 lines of code) both restate "Column definitions are re-decoded into the same slot on every COM_STMT_EXECUTE", with a third repetition at ~line 230. The same cache-policy rationale is also documented three times per driver (field doc oncached_statement_js, the 13-16 linebuild_statement_jsdoc, and the resolve/on_result call-site comment). Suggest keeping the full rationale once on thecached_statement_jsfield doc and collapsing the rest to one-line issue links, e.g.// oven-sh/bun#28632: skip re-copy when unchanged so repeated executions stay allocation-flat.Extended reasoning...
What the issue is
REVIEW.md ("Code style & idioms reviewers enforce") states:
Only comment what the code cannot say. One line. Never restate what the code does. Never narrate the change. Prefer links to GitHub issues.
CLAUDE.md #13/#14 add: "If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong" and "Is this information the next Claude would spend multiple tool calls trying to understand? If not, delete it."
The repo's own
comment-copGitHub Action has posted 14 unresolved inline comments on this PR (2026-08-14) at exactly these locations, each quoting CLAUDE.md #13. Independent verification confirms each flagged block is a multi-line prose comment that either narrates the change or duplicates rationale documented elsewhere in the same PR.The worst case: ColumnDefinition41.rs:121-143
Two consecutive comment blocks preceding
let table = reader.encode_len_string()?;:- Lines 121-129 (9 lines): explains
changedtracks whether any field surfaced inresult.columnsdiffers, that "Column definitions are re-decoded into the same slot on every COM_STMT_EXECUTE / result set", and gives theSELECT 1 AS x; SELECT 'hi' AS xexample plus thetest/regression/issue/28632link. - Lines 132-143 (12 lines): explains why
name/tableare owned copies, then repeats "Column definitions are re-decoded into the same slot on every COM_STMT_EXECUTE of a reused prepared statement" and re-citestest/regression/issue/28632. - Lines ~226-235 (pre-existing on main, 10 lines): a third block repeating "The server re-sends column definitions on every COM_STMT_EXECUTE, so a reused prepared statement re-decodes into the same slot once per query… shows up as steady RSS growth under the ASAN quarantine (test/regression/issue/28632)."
That's ~21 lines of new comment for ~5 lines of code, restating the same fact three times within one function.
The duplication is also cross-file
The cache-policy rationale ("built once per prepared statement, held via a Strong reference, reused across executions, invalidated when column definitions change, never cached for text-protocol/simple queries because pinning would keep large query strings alive") is documented in three places per driver:
Location MySQL Postgres Field doc on cached_statement_jsMySQLStatement.rs:23-29(7 lines)PostgresSQLStatement.rs:22-26(5 lines)build_statement_jsfn docJSMySQLQuery.rs:237-252(16 lines)PostgresSQLQuery.rs:294-306(13 lines)Call-site comment in resolve/on_resultJSMySQLQuery.rs:356-361(6 lines)PostgresSQLQuery.rs:403-408(6 lines)Other flagged sites:
FieldDescription.rs:8-10, 33-34;MySQLConnection.rs:1274-1276;PostgresSQLConnection.rs:2537-2543, 2628-2629;PostgresSQLQuery.rs:336-338.Step-by-step proof
grep -n 're-decoded into the same slot' src/sql/mysql/protocol/ColumnDefinition41.rs→ hits at lines ~123, ~139;grep -n 're-decodes into the same slot'→ hit at ~230. Three occurrences of the same rationale in one function.grep -n 'test/regression/issue/28632' src/sql/mysql/protocol/ColumnDefinition41.rs→ 3 hits in one file; also cited inMySQLStatement.rsandJSMySQLQuery.rs.- The
build_statement_jsdoc comment atJSMySQLQuery.rs:237-252is 16 lines; the field doc it duplicates atMySQLStatement.rs:23-29already says the same thing in 7. - The PR timeline shows 14 unresolved
github-actionscomment-copinline comments dated 2026-08-14, one per flagged block — the repo's own automation agrees.
Why existing code doesn't prevent it
Nothing in the build or CI enforces comment length;
comment-coponly posts inline warnings. The blocks were written incrementally in response to earlier review threads (each fix commit added another explanatory paragraph rather than editing the existing one), which is how the same sentence ended up written three times.Impact
Zero runtime impact — this is comment hygiene only. But it violates an explicit repo rule enforced by an automated check, and the duplication makes the file harder to maintain: the next person editing the invalidation logic has to keep three prose blocks in sync (they've already drifted once — c3b26dc had to fix a stale
to_js()reference in one of them).Suggested fix
Keep the full rationale in exactly one place — the field-level
///doc oncached_statement_jsin{MySQL,PostgresSQL}Statement.rs— and collapse each of the other 12 sites to a single line linking the issue, e.g.:// oven-sh/bun#28632: skip re-copy when unchanged so repeated executions stay allocation-flat. let table = reader.encode_len_string()?;
For
ColumnDefinition41.rsspecifically, the two adjacent blocks at 121-143 can become one line each ("changedreports whether anyresult.columnsfield differs — seeMySQLStatement::cached_statement_js" and "own a copy:Data::Temporaryslices into the read buffer are invalidated beforebuild_statement_jsreads them"), and the third block at ~226-235 can shrink to "skip rebuild when unchanged (#28632)". - Lines 121-129 (9 lines): explains
|
Addressed in a8c1c4b (pushed just before this review ran against 31e779f): every comment block this PR adds under The third block mentioned around line 226 (the |
…ecode The row structure and duplicate check depend only on name_or_index, while the cached statement object depends on the raw name, table, type, length and flags. Returning the two signals separately keeps a type-only change from rebuilding the row structure, and a duplicate-name re-execution from rebuilding the statement object. The prepare path only ever fills freshly allocated slots, so it has nothing to invalidate.
|
Heads-up from #38443: the existing |
2 participants
Fixes #26809. Fixes #18866. Supersedes #18892.
Problem
Bun.sqlauto-parses query results (JSONB → JS objects, arrays → JS arrays) but doesn't expose the underlying PostgreSQL column type OIDs. This makes it impossible to distinguish between:jsonbcolumn containing["a","b"](should be treated as JSON)text[]column containing["a","b"](should be treated as a text array)Both produce identical JS arrays. Driver adapters (e.g. Prisma) need the wire-protocol column types to handle these correctly.
What this adds
Query results now carry
.columnsand.statement:Works for tagged templates,
.unsafe(),.values(),.raw(), and.simple()(including multi-statement queries, where each result set gets its own columns).PostgreSQL —
{ name, type, table, number }(type = OID frompg_type, matchespostgres.js'sresult.columns).MySQL/MariaDB —
{ name, type, table, length, flags }(type = protocol column-type code).SQLite —
null(no wire-protocol row description).Implementation
Rather than a getter on the native query handle (PR #18892's approach, which has lifetime issues when a simple query's
RowDescriptionis overwritten between result sets), the column metadata is built inonResult/resolveand passed as an extra argument to the JS resolve callback, which stores it directly on theSQLResultArray. This makes multi-statement.simple()queries return correct per-result-set columns.FieldDescriptionnow stores the raw column name alongsidename_or_index, so duplicate column names survivecheckForDuplicateFields()and are reported correctly in.columns(matchingpostgres.js).The
{ string, columns }object is built once per statement and held on the native statement via a Strong reference (same lifetime/invalidation policy ascached_structure), so re-executing a prepared statement reuses it instead of rebuilding per query — matchingpostgres.js, wherestatementis the shared prepared-statement descriptor. It is invalidated wherever the statement's fields are cleared or replaced (simple-protocol batches, result-set column-count changes). On MySQL, where the server re-sends column definitions on every execution,ColumnDefinition41::decodereports what a re-decode changed asChanged { structure, metadata }:structure(the pre-existingname_or_indexsignal) invalidates the row structure and duplicate check, whilemetadata(raw name, table, type, length, flags) invalidates the statement object, so equal-width result sets that differ only in type get fresh metadata without rebuilding the row structure. Together with skipping theColumnDefinition41name/table re-copy when the re-decoded bytes are unchanged, repeated executions stay allocation-flat: without this, the per-query churn pushedtest/regression/issue/28632.test.ts(MySQL RSS regression test) from ~7 MB to ~44 MB growth on the x64-asan lane (threshold 36 MB); with it, a local run of the same 5000-query loop against MariaDB shows no measurable growth over the no-feature baseline.Verification
test/js/sql/postgres-result-columns.test.ts— mock Postgres wire-protocol server (no database required): name/type/table/number incl. negative attnums, zero-row result sets, duplicate names, per-result-set columns for multi-statement.simple(), and no stale-column inheritance for commands without aRowDescription. Fails without the native changes, passes with them.test/js/sql/sql.test.ts/test/js/sql/sql-mysql.test.ts(docker-gated) — jsonb vs text[] OIDs, table OIDs,.unsafe()/.values()/.raw()/.simple(), long (>15 byte) column-name ownership, andresult.statement/result.columnsobject reuse across re-executions of the same prepared statement.Rebase notes
test(sql): centralize fault-injection wire frames; convert decode mocks to real servers #32467 converted
sql-mysql-query-string-leak.test.tsfrom a mock TCP server to a realdescribeWithContainer("mysql")test; the warmup-batch change (snapshot the RSS baseline after a short identical workload so the measured delta isolates retained strings rather than first-touch heap growth) is carried forward inside the new container-based fixture.test(sql): centralize fault-injection wire frames; convert decode mocks to real servers #32467 also centralized Postgres/MySQL wire-frame builders into
test/js/sql/wire-frames.ts;postgres-result-columns.test.tsis rewritten to usepgRowDescription/pgDataRow/pgCommandComplete/pgReadyForQuery/listeningServerinstead of its own frame encoders. It stays a mock-server test so the RowDescription→result.columnsmapping (including negative system-column attnums) is verified byte-for-byte without needing a container; the real-server coverage is insql.test.ts.Rebased over the MariaDB extended-type-info work (sql(mysql): negotiate MariaDB extended type info so JSON columns parse into objects #37130):
ColumnDefinition41::decodenow takesextended_type_info, and main remapsformat=jsonTEXT/BLOB columns toMYSQL_TYPE_JSONduring decode. The remap is applied to the decoded type before this PR's change-detection compare, soresult.columns[i].typereports the same type code the row decoder uses (245 for MariaDB JSON columns, verified against MariaDB 11.8) and a column flipping to or from JSON format invalidates the cached metadata like any other type change.Rebased over the dead-code and visibility-narrowing passes:
FieldDescription.table_oid/column_indexhad been removed on main as unread and are restored here since this PR is their consumer;ColumnDefinition41.name/tablearepubagain becausebun_sql_jscreads them; the removedColumnFlags::to_intis replaced by the bitflagsbits()accessor (samefrom_bits_retainround-trip of the raw wire value);cached_statement_jsand the new accessors follow main'spub(crate)convention.The leak test keeps main's
rss()measurement helper (test: measure memory via harness rss() instead of process.memoryUsage.rss() #36429,memoryFootprinton macOS) together with this PR's warmup batch.no test proof · iteration 37 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/sql/sql-mysql-query-string-leak.test.ts