sql(postgres): route the six Bind encoder call sites through PostgresSQLConnection::encode_request; pin Bind wire bytes - #43892
Conversation
|
Status: rebased on main at 8884311 (after #43898 merged), head ec7e250. The rebase had no conflict. The full This PR changes no behaviour, so it has no failing-before test. Next in the stack: #43918 (a query dispatched while parameters are converted only enqueues), then conversion before the first write. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the refactor for behavior drift: the four advance() arms keep their old error-path side effects (statement marked Failed only at the two first-Parse sites, JS exception path unchanged), and the now-unused connection.writer() in do_run before encode_request is harmless — NewWriter's truncate-on-error guard captures the epoch after encode_request's own bump, so a failed batch still truncates correctly.
Extended reasoning...
The change routes the six Bind encoder call sites in src/sql_jsc/postgres/PostgresSQLConnection.rs and PostgresSQLQuery.rs through a new PostgresSQLConnection::encode_request in PostgresRequest.rs, makes the three batch writers private, extracts reject_failed_encode, and adds a mock-server test pinning Bind frontend bytes. It touches no auth, crypto, or injection surface; parameters are still encoded through the unchanged write_bind. The Rust side is behavior-preserving on inspection, but with inline findings posted and further verified findings withheld, this is not an approve.
71d61bc to
6fcff61
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 8:58 AM PT - Sep 24th, 2026
✅ @robobun, your commit f787b5cb9d8cfd98c820e97ad648c55081259391 passed in 🧪 To try this PR locally: bunx bun-pr 43892That installs a local version of the PR into your bun-43892 --bun |
There was a problem hiding this comment.
I re-reviewed the updated PR and found no bugs; the earlier inline feedback (file header, pgRaw instead of a local frame builder, a reachable int8 case, and routing the execute_query/write_query/SYNC arms through the helper) is addressed in the code. Because this rewires every Bind encode path inside advance() and I could not run bun bd test here (no debug build in this checkout), a human look at the Rust side is still worthwhile.
What was reviewed:
encode_requestobtainsself.writer()(which bumpswrite_epoch) beforeatomicallyrecords the epoch, so the failed-batch rollback from the base commit still triggers; the extrawriter()call left inPostgresSQLQuery::runfor the simple-query/write_query path is harmless for the same reason.reject_failed_writearguments match the old arms one-to-one (Nonefor the two arms that never marked the statement,Some(statement)for the four that did); the swapped order ofstatus/error_responseassignment at the write_query/SYNC arms is unobservable.- Pinned frontend bytes in
wire-frames.test.ts(Parse 0x17, Describe 0x08, Bind 0x20, Execute 0x09) check out against the §55.7 layouts. - The "Above i16::MAX" comment at
test/js/sql/postgres-bind-wire.test.ts:35is off by width (the client's cutoff inwrite_bindisShort::MAX, i.e. u16), but 70000 exceeds both, so the case still exercises the unknown-type path.
Extended reasoning...
The diff touches three Rust files in src/sql_jsc/postgres (a new EncodeRequest enum and encode_request method, a reject_failed_write helper used at seven error arms in advance(), and two call sites in PostgresSQLQuery::run) plus a new mock-server wire test and frontend frame builders; no security-sensitive surface (no auth, TLS, or user-input parsing changes) is involved. The Rust change is mechanical and I traced it as behavior-preserving, and all of my prior inline objections were addressed by commits 6fcff61 and d90e979. I deferred rather than approved because the change rewires the core Bind path of the Postgres driver state machine and the tests could not be executed in this checkout (no debug build), so the author's claim that all postgres tests pass is unverified here.
This review covers commit d90e979, which is no longer the latest commit on this pull request; later commits are not covered by it.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughPostgreSQL request encoding now uses connection-level dispatch, and request writers serialize protocol messages atomically. Connection write failures use shared handling. New tests check frontend frame bytes and named and unnamed statement batches. ChangesPostgreSQL Request Encoding
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable behavior change was established; the PR is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
c2d8b0c to
f787b5c
Compare
There was a problem hiding this comment.
LGTM — the new commits address everything raised earlier (all seven advance() arms now go through reject_failed_write, the local frame builder is gone in favour of pgRaw, and the int8 case now declares the type it describes), and the refactor is behaviour-preserving.
What was reviewed:
- Each of the seven
reject_failed_writesites against its pre-diff arm: theNone/Some(&mut *statement)choices match which arms previously setstatus = Failed+error_response, andtry_take_exceptionstill runs before any statement mutation. - The extra
self.writer()insideencode_requeston thedo_runpath: it bumpswrite_epochbeforeatomicallycaptures the epoch, so the partial-Bind rollback from #34732 is unaffected; thewriterlocal is still live forwrite_query/SYNC. - Grepped for other callers of the now-private batch writers — none outside PostgresRequest.rs (the MySQL
bind_and_executeis a separate method). - Test isolation: the client sends no Terminate on close, so the shared
receivedmap cannot leak a frame from the firstdescribeinto the second'sdrain().
Extended reasoning...
The diff is a mechanical routing refactor in the Postgres client: three batch writers and write_bind become private to src/sql_jsc/postgres/PostgresRequest.rs, a new EncodeRequest enum plus PostgresSQLConnection::encode_request is the single entry point, six call sites in PostgresSQLConnection.rs advance() and PostgresSQLQuery.rs do_run switch to it, and seven identical error tails collapse into reject_failed_write. It touches no security-sensitive surface (no auth, TLS, or parsing of server bytes changes; parameter encoding itself is untouched). No CODEOWNERS entry covers the changed files, no CHANGES_REQUESTED review is outstanding, and the four new commits address every point from the two prior review rounds. The new test file pins frontend bytes for the named, unnamed, sql() helper and no-parameter paths against a local mock and passes on main, so it guards the refactor rather than a new behaviour. Approving because I traced each arm's old versus new side effects and found them identical.
…SQLConnection::encode_request; pin Bind wire bytes The three batch writers that encode Bind parameters (bind_and_execute, prepare_and_query_with_signature, parse_and_bind_and_execute) were called from four places in advance() and two in do_run. They are now private to PostgresRequest.rs and reached only through PostgresSQLConnection::encode_request. advance() rejects a request whose batch failed to encode through one function, reject_failed_encode, in place of four copies of the same arm. No behaviour change. test/js/sql/postgres-bind-wire.test.ts pins the exact frontend bytes for each kind of parameter (named statements, prepare: false, no parameters, the sql() helper), built from new frontend message builders in wire-frames.ts.
…eachable int8 case, pgRaw, header The simple-query arm and the two arms of the named Parse/Describe/Sync write carried the same error tail as the four Bind arms. All seven now go through reject_failed_write. The corpus described an int8 parameter that Parse declared as int4, which a real server cannot answer. It now binds a BigInt (declared and described as int8). The file header says why a mock is used, and the recorder uses pgRaw.
f787b5c to
ec7e250
Compare
2 participants
Behaviour change: none
Part 1 of 3 of the follow-up to #34732 (plan). Rebased on main after #43898 merged.
Problem
PostgresSQLConnection::advanceand two inPostgresSQLQuery::do_run. Each calls one of three batch writers inPostgresRequest.rsdirectly, andadvancecarries seven copies of the same error arm.Fix
PostgresSQLConnection::encode_request(global, EncodeRequest)is now the only route tobind_and_execute,prepare_and_query_with_signatureandparse_and_bind_and_execute. They andwrite_bindare private toPostgresRequest.rs.advancerejects a request whose write failed through one function,reject_failed_write, at all seven arms. The one difference between the old arms (a statement's first write marks the statement failed) is itsnew_statementargument.postgres-bind-wire.test.tspins the exact frontend bytes for 13 kinds of parameter, a binary result column, thesql()helper, no parameters, andprepare: false. It passes on main without this PR.wire-frames.tsgets frontend builders with a layout self-test.sql.test.ts,sql-prepare-false.test.ts,postgres-prepared-pipeline-reorder.test.ts,postgres-split-prepare-reorder.test.ts,postgres-simple-query-pipeline.test.ts,postgres-bytea-bind.test.ts,wire-frames.test.ts. All 255 tests intest/js/sql/postgres-*pass on the debug build.Background
toString,toJSON, getters).float4,numeric,time,int4[]). sql(postgres): announce binary format for a Bind parameter only when it is binary-encoded #41976 fixes those.Downsides
encode_requestis not inlined. No allocation, no syscall.[human-review] gate passed · iteration 3 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 3
evidence per changed file