sql: collapse repeated const tables into macros and drop dead protocol traits - #32128
Merged
Conversation
Collaborator
|
Updated 1:01 PM PT - Jun 11th, 2026
❌ @alii, your commit a0e184e has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32128That installs a local version of the PR into your bun-32128 --bun |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (9)
WalkthroughThis PR removes internal Postgres protocol wrapper traits ( ChangesPostgres Protocol Wrapper Removal
Code Generation Macro Consolidations
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
alii
added a commit
that referenced
this pull request
Jun 11, 2026
…gres and mysql (#32135) ## What this does Second slice carved out of #31994 (closed as too big), stacked on #32128 (merged). This one is the `src/sql_jsc/` Rust constructor-args dedup — no JS adapter changes. Net −58 lines across 20 files. Three commits: - **`src/sql/` prerequisites** (46d3fb8) — `QueryStatus` moves from `mysql/` to `shared/` (re-exported under the old `mysql::query_status` path so nothing breaks); new `shared/StatementStatus` with the same 4 variants both drivers already had; `FieldMessage` gains a `payload()` accessor so the only caller no longer needs an external exhaustive match; `NoticeResponse` becomes a type alias of `ErrorResponse` decoded via `decode_notice_internal` (same wire format, body identical to the old `NoticeResponse::decode_internal`). - **`src/sql_jsc/` dedup** (dc96ffd) — the headline change. Extracts the duplicated `createQuery(...)` and `createConnection(...)` argument parsing/validation prologue from `JSMySQLQuery` / `PostgresSQLQuery` and `JSMySQLConnection` / `PostgresSQLConnection` into `src/sql_jsc/shared/{query_ctor_args,connection_ctor_args}.rs`. Also moves the duplicated `dedupe_columns` from `MySQLStatement` / `PostgresSQLStatement` into `shared/SQLDataCell.rs`, switches both Statement types to the shared `StatementStatus`, and drops the now-unused `notice_response_jsc.rs` (replaced by `FieldMessage::payload()`). - **Test coverage for the rewired NoticeResponse path** (159cea4) — adds `NoticeResponse` and degenerate empty-notice cases to the existing async-message framing test in `test/js/sql/postgres-multi-statement-fields.test.ts`. This path had no prior coverage in `test/js/sql`. ## Behavioral equivalence Line-by-line diff of every removed per-driver block against the new shared helper: - All five `query_ctor_args` error messages verbatim (`"query must be a string"`, `"values must be an array"`, `"simple query cannot have parameters"`, `"query is too long"`, `throw_invalid_argument_type("query", "pendingValue", "Array")`); validation order unchanged. - `connection_ctor_args` validation order unchanged; `ssl_mode` decode equivalent for all i32 (negative → Disable, 0–4 → indexed, ≥5 → Disable); `tls` error message verbatim; same per-VM `SSL_CTX*` cache and `as_usockets_for_client_verification()` key; same `SSL_CTX_free` symbol. - `dedupe_columns` iteration order and reserve size identical. `.expect("OOM")` → `.unwrap_or_oom()` routes through `bun_core::handle_oom` per repo convention — same crash on OOM, no observable difference otherwise. - `bun_vm()` vs `sql_vm()`: the latter is `#[inline] fn sql_vm(&self) { self.bun_vm() }`. No user-reachable input produces a different output, error message, validation order, or SSL_CTX cache key. ## Verification - `cargo check -p bun_sql -p bun_sql_jsc` and `cargo clippy --no-deps`: clean, zero warnings. - `cargo fmt`: no diffs. - `bun bd test` mock-server SQL suite (`postgres-binary-numeric`, `postgres-binary-array-bounds`, `postgres-multi-statement-fields`, `sql-mysql-auth-short-nonce`, `sql-connect-error-reporting`): 41 pass / 0 fail (39 prior + 2 new NoticeResponse cases). - No commits on `main` have touched `src/sql_jsc/` since #31994's merge-base, so the per-driver files are a clean checkout with no clobbered work. ## Left for a follow-up The `src/js/internal/sql/shared.ts` JS adapter consolidation from #31994 (the `BasePooledConnection` extraction, +1308/−2273) is intentionally not in this PR — that's the higher-risk piece touching pool/connection lifecycle and the part that conflicted with #32028.
alii
added a commit
that referenced
this pull request
Jun 11, 2026
## What this does Third (and last Rust) slice from #31994, on top of #32128 and #32135. Consolidates the per-driver `StackReader` cursor type into `src/sql/shared/StackReader.rs`. Net −29 lines across 6 files. `StackReader` is the cursor-over-borrowed-buffer that both drivers' `NewReader` types wrap on the per-row decode path. The MySQL and Postgres copies were near-identical; the only per-driver bits were the error variant returned on a short read and the `NewReader<C>` wrapper type. Those become two small traits (`ShortRead`, `WrapReader`) that each driver implements in ~10 lines; the per-driver `StackReader.rs` files are now those impls plus a re-export. Also drops two dead items from `mysql/protocol/NewReader.rs`: the `NewReaderOf<C>` type alias (= `NewReader<C>`, zero callers) and `Decode::decode_allocator` (body identical to `decode`, zero callers). ## Behavioral equivalence - MySQL: byte-for-byte semantic match. Old already used `&Cell<usize>`; `skip` negative-count branch via `saturating_sub` is identical to the old explicit clamp. - Postgres: cursor was `&mut usize`; new shared uses `&Cell<usize>` for both. `IntoCursor for &mut usize` is `Cell::from_mut` (a `repr(transparent)` pointer cast), so the existing `&mut consumed`/`&mut offset` call site at `PostgresSQLConnection.rs:982` is unchanged and reads back the same locals after the reader is consumed. - `ShortRead::SHORT_READ` monomorphizes to the literal `AnyMySQLError::ShortRead` / `AnyPostgresError::ShortRead` — identical `Err` values to before. Two strictly-safer deltas on the postgres side, both unreachable on real wire data but the correct direction: - `ensure_length`/`ensure_capacity` use `checked_add` where the old `buffer.len() >= offset + length` could overflow-wrap in release on adversarial declared lengths. - `skip(count > isize::MAX)` clamps to buffer end instead of overflow-wrapping. ## Performance No new dynamic dispatch or boxing — `ShortRead`/`WrapReader`/`IntoCursor` are all generic-bound. `Cell::get/set` on `usize` compiles to plain load/store. The per-row hot path (`DataRow` → `NewReader::int` → `wrapped.read(N)`) is slightly leaner: shared `read` advances the cursor with `offset.set(offset + count)` instead of routing through `skip()` with its redundant bounds re-check. The one added `isize::try_from().unwrap_or()` in the postgres `skip` bridge is not on the row path (`skip` is called only from `FieldDescription` once per column definition and `Authentication` once per connection). ## Verification - `cargo check -p bun_sql -p bun_sql_jsc` and `cargo clippy --no-deps`: clean. - `cargo fmt`: no diffs. - Zero callers of `NewReaderOf` / `decode_allocator` via grep. ## Left for a follow-up The `src/js/internal/sql/shared.ts` JS adapter consolidation from #31994 (`BasePooledConnection`, +1308/−2273) — the higher-risk pool/lifecycle piece.
alii
added a commit
that referenced
this pull request
Jun 12, 2026
…n in shared.ts (#32145) ## What this does Final slice from #31994 (closed as too big), on top of #32128, #32135, #32141. Consolidates the JS adapters' duplicated pool/connection/query plumbing from `src/js/internal/sql/{postgres,mysql,sqlite}.ts` into `src/js/internal/sql/shared.ts` as a `BasePooledConnection` class hierarchy. Net −995 lines across 4 files. Two commits: - **Cherry-pick from #31994** (1aa3dbb) — checks out `src/js/internal/sql/` from `origin/claude/split/sql` as-is. This intentionally clobbers two pool-lifecycle fixes that landed on `main` since #31994's merge-base: #32041 (throwing onconnect/onclose corrupting the pool) and #32097 (forced `close()` resolving mid-handshake). - **Re-integrate #32041 + #32097 into the new structure** (6e6fd4f) — re-hosts both fixes in `BasePooledConnection`: - #32041: try/finally around the user `onconnect`/`onclose` calls in `handleConnected` and `#finishClose` so a throwing callback never skips pool bookkeeping; the `createPooledConnectionHandle` catch always defers via `process.nextTick` (drops the per-driver `deferSyncCloseError` flag — both drivers now match). - #32097: `startConnection()` is `abstract Promise<void>` and both subclasses await + assign `this.connection` at creation; `#beginConnecting` awaits it and closes the handle if `onFinish` was set (pool force-closed) in the microtask window before it materialized. ## Behavioral equivalence The original `BasePooledConnection` extraction in #31994 carried a per-file behavioral-equivalence audit against its merge-base (placeholders, escaping, helper commands, error message text, pool/transaction semantics all preserved) and was green on full CI build 61383. This PR re-applies that diff and adds the two missing fixes; the load-bearing verification is below. ## Verification - `bun bd`: builds clean. - The three lifecycle test files (`sql-onconnect-onclose-throw.test.ts`, `sql-close-pending-connection.test.ts`, `sql-connect-error-reporting.test.ts`): 26 pass / 0 fail. - **New tests in this PR**: `sql-onconnect-onclose-throw.test.ts` gains a forced-close variant per driver (a throwing `onclose` while the pool is force-closed mid-handshake, against a fake `net` server). The two re-hosted fixes meet in `BasePooledConnection`'s close handler, and that interaction had no coverage. Note these pass against `origin/main`'s `src/js/internal/sql` too (verified): both underlying fixes already live on main per-driver, so versus main this PR is equivalence-preserving by design and no test can fail on one side only. The HEAD~1 stash test below is the proof that the re-integration commit is load-bearing. - **Load-bearing stash test**: built and ran the same three files at HEAD~1 (the cherry-pick before re-integration) — 6 fail (`mysql: forced close() resolves when called before the native handle is stored` timeout; `postgres: pool calls from onclose are safe when connecting fails synchronously` → `reentry threw: TypeError`; both drivers' `throwing onclose still rejects pending queries on connect refused` timeouts). With the re-integration: 26 pass. The diff is required for both fixes. - Full `test/js/sql/` against live `postgres_plain`/`mysql_plain`/`*_tls` containers: 1526 pass / 7 fail / 6 errors locally. Every failure reproduces on `origin/main` in the same environment — see Notes below. ## Notes on local-only test failures (none introduced by this PR) - `sql-mysql-query-string-leak.test.ts` fails locally on a debug+ASAN build for both `origin/main` (314.6 MiB) and this branch (317.2 MiB), within noise of each other; both exceed the 256 MiB ASAN threshold. A `heapStats()` probe of the same workload shows `MySQLQuery 2→1` after GC — wrappers are finalized correctly. The test passes on `main`'s CI (release build); the threshold is tuned for release RSS overhead, not local debug. - `sql.test.ts > query string memory leak test` (postgres): same category. - `sqlite-sql.test.ts > Query Normalization Fuzzing Tests > handles exotic but valid SQL patterns`: 5.5s timeout on debug builds — pre-existing per #31994's original verification notes. - `describeWithContainer` cold-start races: `docker compose up -d --wait` returns non-zero on an already-healthy container (`test/docker/index.ts:157`), causing beforeEach hook timeouts in `sql-mysql.test.ts` (TLS), `sql-mysql-bind-oob`, `sql-mysql.helpers`, `sql-mysql.auth`. Pre-existing harness flake; clears on warm rerun. The Rust-side changes from #32097 (`src/sql_jsc/`) were already on `main` before this PR — only the JS hooks needed re-integrating.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Smaller, lower-risk replacement for #31994 (closed as too big). Scope is the
src/sql/Rust protocol crate only — no JS adapters, nosrc/sql_jsc/bindings. Net −686 lines across 12 files.Five independent changes, each its own commit:
DecoderWrap/WriteWrap(200fcea, a0e184e) — postgres protocol trait + struct with zero implementors and zero callers anywhere insrc/(vestigial Zig-port wrappers). Removed both files plus their re-exports, the three unreferenced_DecoderWrapTargetaliases, and the stale comment inCommandComplete.rsthat pointed at the deleted file.Capabilities5× flag list into one macro (8c1e6c7) — the same 32(name, bit)pairs were hand-maintained five times (struct fields, private bit consts,to_intif-chain,from_intliteral,Displayemit list). Onecapabilities! { NAME = bit, ... }macro now generates all of it.to_intbecomes a branchless(bool as u32) << bitOR-fold;from_int/Displayexpand to byte-identical code.reject/intersect/get_default_capabilitiesare unchanged in a separateimpl.CharacterSetconsts +label()from one list (0fcb0b8) — 223pub const NAME = CharacterSet(N)lines and a separate 99-armlabel()match re-pairing the sameN => name. Onecharacter_sets! { name = id, ... }macro now emits both from a single list.DEFAULTalias andDefaultimpl unchanged.mysql_types::CharacterSetenum (74c7b24) — the exhaustive 223-variant#[repr(u8)]enum inMySQLTypes.rshad zero callers; every consumer (SSLRequest,HandshakeV10,HandshakeResponse41) uses the liveprotocol::character_set::CharacterSetnewtype instead. The enum's own header comment already noted nothing decodes into it.Tagconsts +tag_name()into one macro (e3ed505) — 89pub const name: Tag = Tag(OID)declarations plus a parallel 89-armtag_name()match. Onepg_tags! { name = oid, ... }macro now emits both.is_binary_format_supported/format_codemoved to a separateimpl Tagunchanged.All 89
Tagconsts and all 223CharacterSetconsts verified preserved by name-set diff againstmain.Behavior notes (debug-log only, dead-stripped in release)
Tag::tag_name()arms now returnstringify!($name).trim_start_matches("r#")instead of a direct literal (handlesr#box). LLVM does not const-foldtrim_start_matchesat-O, so each call allocates a small stackStrSearcher. The sole caller isbun_core::scoped_log!(Postgres, ...)atsrc/sql_jsc/postgres/PostgresRequest.rs:154, which is dead-stripped in release builds.CharacterSet::label()now covers ids 100–250 (previously returned"(unknown)"for those, since the old hand-written match only listed ids 1–99 while 223 consts existed). Sole caller isbun_core::scoped_log!(MySQLConnection, ...)atsrc/sql_jsc/mysql/MySQLConnection.rs:657— debug log output only.Verification
cargo check -p bun_sql -p bun_sql_jsc: clean, zero warnings.cargo clippy -p bun_sql -p bun_sql_jsc --no-deps: clean.cargo fmt: no diffs.Tag(89),CharacterSet(223), andCapabilitiesbit positions (32) againstmain— all identical.bun bd teston the mock-server SQL tests that exercise the changed tables without DB containers —postgres-binary-numeric,postgres-binary-array-bounds,postgres-multi-statement-fields,sql-mysql-auth-short-nonce,sql-connect-error-reporting: 39 pass / 0 fail.Declined from review
Capabilitieson top of the workspacebitflags!crate — would change ~15 field-access sites (self.CLIENT_SSL→self.contains(...)) and the public bool-field API. Out of scope for a tidy PR.trim_start_matches("r#")with an optionalas $labelmacro arm — extra grammar for one entry on a debug-only path.