sql: consolidate per-driver StackReader into shared/

[alii]

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.

[robobun]

^{Updated 4:23 PM PT - Jun 11th, 2026}

❌ @alii, your commit 5e04e03 has 1 failures in Build #61961 (All Failures):

• test/js/third_party/@duckdb/node-api/duckdb.test.ts - pid 11173 segmentation fault on 🐧 13 x64-baseline

---

🧪   To try this PR locally:

bunx bun-pr 32141

That installs a local version of the PR into your bun-32141 executable, so you can run:

bun-32141 --bun

[coderabbitai]

Walkthrough

This pull request extracts buffer cursor management into a shared StackReader abstraction, moving duplicate logic from MySQL and Postgres protocol modules into a unified implementation. Both protocol adapters now delegate reading and cursor operations to the shared type while retaining protocol-specific error handling and trait wrapping via pluggable interfaces.

Changes

Shared StackReader Extraction and Protocol Integration

Layer / File(s)	Summary
Shared StackReader traits and struct definition src/sql/shared/StackReader.rs	ShortRead, WrapReader, and IntoCursor traits parameterize error types, wrapping behavior, and cursor sources. The StackReader<'a> struct holds a buffer and Cell-backed cursors, with an init factory that constructs and passes the reader to a protocol-specific wrapper.
Shared StackReader read and cursor operations src/sql/shared/StackReader.rs	ensure_capacity, peek, and skip handle read-ahead and cursor movement with signed clamping. read checks bounds and returns a temporary slice view; read_z scans for zero terminators and advances past them or returns a SHORT_READ error.
MySQL StackReader adapter and ReaderContext delegation src/sql/mysql/protocol/StackReader.rs	Re-exports shared StackReader, implements ShortRead for AnyMySQLError and WrapReader for NewReader<StackReader<'a>>. ReaderContext methods delegate to shared implementation.
Postgres StackReader adapter and ReaderContext delegation src/sql/postgres/protocol/StackReader.rs	Re-exports shared StackReader, implements ShortRead for AnyPostgresError and WrapReader for NewReader<StackReader<'a>>. ReaderContext delegation includes signed conversion of skip count to preserve unsigned clamping semantics.
NewReader API simplification and refactoring src/sql/mysql/protocol/NewReader.rs, src/sql/postgres/protocol/NewReader.rs	MySQL removes NewReaderOf type alias and decode_allocator trait method, adds From<C> for into() construction. Postgres clarifies the blanket NewReaderWrap<&mut C> reborrow implementation comment.
Shared module introduction and re-export updates src/sql/lib.rs	Adds shared::stack_reader module and re-exports StackReader at shared level. Removes NewReaderOf from MySQL protocol re-exports.

Suggested reviewers

• cirospaciari

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: consolidating per-driver StackReader implementations into a shared module. It directly matches the primary objective of the PR.
Description check	✅ Passed	The description fully addresses the template requirements with detailed explanations of what the PR does and how verification was performed, though it goes beyond the minimal template structure.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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 `@src/sql/shared/StackReader.rs`:
- Around line 62-64: The cursor math currently uses unchecked addition and an
expect-based int cast which can panic; update set_offset_from_start (and the
other shared-reader paths that add offsets around the code handling
message_start + offset and the additions at lines ~85-91) to perform bounds-safe
arithmetic (e.g., use checked_add or saturating_add and then clamp to
buffer.len() or return a Result instead of panicking) and replace the
expect("int cast") at the cast site with a fallible conversion (usize::try_from
or to/from conversions returning a Result) and handle the Err case without
unwrap (clamp, return Err, or no-op) so all external-data index/length math
maintains offset <= buffer.len() and never panics; reference the fields/methods
message_start, offset.set, and the specific int-cast site when making changes.

🪄 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: 1cfa5fea-9f93-4519-8852-ab321de68a6d

📥 Commits

Reviewing files that changed from the base of the PR and between 170fcd7 and 5e04e03.

📒 Files selected for processing (6)

• src/sql/lib.rs
• src/sql/mysql/protocol/NewReader.rs
• src/sql/mysql/protocol/StackReader.rs
• src/sql/postgres/protocol/NewReader.rs
• src/sql/postgres/protocol/StackReader.rs
• src/sql/shared/StackReader.rs

💤 Files with no reviewable changes (1)

• src/sql/mysql/protocol/NewReader.rs

[claude]

I didn't find any issues — the Cell::from_mut roundtrip for the postgres caller checks out and the dead-code removals have zero remaining references — but since this swaps the cursor representation on the per-row decode hot path for both SQL drivers, it's worth a quick human look.

Extended reasoning...

Overview

Consolidates the two per-driver StackReader cursor types (mysql + postgres) into a single shared implementation in src/sql/shared/StackReader.rs, parameterized by three small traits (ShortRead for the error variant, WrapReader for the NewReader<C> wrapper, IntoCursor to accept either &Cell<usize> or &mut usize). The per-driver StackReader.rs files become thin trait impls + a re-export. Also removes two dead items from mysql/protocol/NewReader.rs (NewReaderOf alias and Decode::decode_allocator). Net −29 lines across 6 files; one comment-only change in postgres/protocol/NewReader.rs.

Security risks

None. This is a pure internal refactor of buffer-cursor arithmetic with no input-handling, auth, or permission changes. The two behavioral deltas on the postgres side (checked_add in ensure_capacity, and skip clamping when count > isize::MAX) move in the strictly-safer direction vs. the old release-mode wrap.

Level of scrutiny

Moderate. The diff is mechanical and the equivalence argument in the PR body is thorough and accurate — I traced the key concern (postgres caller at PostgresSQLConnection.rs:982 passes &mut consumed/&mut offset, reads them back after a short-read; Cell::from_mut is a transparent reborrow so the writes land in the original locals) and it holds. However, this is the per-row wire-protocol decode path for both SQL drivers, the postgres cursor representation changes from &mut usize to &Cell<usize>, and init now relies on return-type inference through a new generic R: WrapReader. That's enough new abstraction on a hot path that a human glance is warranted rather than a bot rubber-stamp.

Other factors

• cargo check / clippy reported clean by the author.
• Verified zero remaining references to NewReaderOf / decode_allocator via grep.
• Verified the two StackReader::init call sites (MySQLConnection.rs:488 with &Cell, PostgresSQLConnection.rs:982 with &mut usize) both map cleanly through IntoCursor.
• No CODEOWNERS entry for src/sql/.
• No prior reviews on this PR; only the robobun build comment in the timeline.